Keyboardio-specific key_defs defined in Kaleidoscope headers

[malsyned]

Kaleidoscope aims to be a keyboard-independent framework, but key_defs.h defines the following Keyboardio-specific Keys:

define Key_LeftParen LSHIFT(Key_9)

define Key_RightParen LSHIFT(Key_0)

define Key_LeftCurlyBracket LSHIFT(Key_LeftBracket)

define Key_RightCurlyBracket LSHIFT(Key_RightBracket)

define Key_Pipe LSHIFT(Key_Backslash)

Not only would it make more sense from a modularity perspective to define these in Model01-Firmware/Model01-Firmware.ino, but in that location I think contributors are more likely to find it when looking for examples of how to create keys which type shifted characters without shift held down.

This would require also defining these in Kaleidoscope/examples/Kaleidoscope/Kaleidoscope.ino, which appears to be a kinda-duplication of Model01-Firmware.ino already anyway.

[malsyned]

I could also imagine these going into Kaleidoscope-Hardware-Model01/src/Kaleidoscope-Hardware-Model01.h to fix the modularity violation

[obra]

Hi!

These aren't Keyboardio-specific keys, so much as an affordance for user-defined key layouts.

There's nothing about them that's tied to the Model 01's hardware and they're something we expect end users to reach for, no matter what hardware they're working with.

Is your concern just the fact that we have Key definitions that aren't a 1 to 1 map to the HID spec?

On Thu, Jul 27, 2017 at 05:50:25PM +0000, malsyned wrote:

I could also imagine these going into Kaleidoscope-Hardware-Model01/src/Kaleidoscope-Hardware-Model01.h to fix the modularity violation

— You are receiving this because you are subscribed to this thread. Reply to this email directly, [1]view it on GitHub, or [2]mute the thread.

Reverse link: [3]unknown

References

Visible links

1. https://github.com/keyboardio/Kaleidoscope/issues/148#issuecomment-318436701
2. https://github.com/notifications/unsubscribe-auth/AACxaD6h0am3ppv2HO29MYIhbCRYoxaiks5sSM3ggaJpZM4OlneM
3. https://github.com/keyboardio/Kaleidoscope/issues/148#issuecomment-318436701

--

[malsyned]

My concern isn't precisely that the key definitions aren't a 1-to-1 map to the HID spec. I think my concern is better explained by example.

Imagining creating a brand new keyboard and basing the firmware on Kaleidescope. Say that this keyboard is a super weird one, and has a row of 5 keys which are !@#$% when un-modified and ^&*() when the Fn key is pressed.

In our new Arduino project, in SomeWeirdKeyboard.ino, one of the things I would do is to define these keys which don't map to unshifted HID scancodes:

define Key_Caret LSHIFT(Key_6)

define Key_Amp LSHIFT(Key_7)

define Key_Asterisk LSHIFT(Key_8)

define Key_LeftParen LSHIFT(Key_9)

define Key_RightParen LSHIFT(Key_0)

because on SomeWeirdKeyboard, those special keys would have to be defined. What I would not do is fork Kaleidescope, add Key_Caret, Key_Amp, and Key_Asterisk to key_defs.h, and send you a pull request.

If key_defs.h is left as-is, when there are 100 keyboards based on Kaleidoscope, 99 of them will have their non-standard keys defined in their .ino files or in a hardware header file, and one -- Keyboardio -- will have its predefined in Kaleidoscope's key_defs.h.

If Kaleidescope had existed in its current form before you started basing Keyboardio on it, and had a separate maintainer, you would have put the five definitions in Model01-Firmware or in Kaleidoscope-Hardware-Model01, not in key_defs.h. The only reason they are in key_defs.h instead is because Kaleidoscope started off as the Keyboardio firmware.

Does that make my concern clearer?

[malsyned]

(and thanks for taking the time to respond to what may seem like a minor quibble)

[obra]

That example is...a bit of a stretch. (Two of your desired new keycodes are, in fact, keycodes we already provide ;)

Given that one of the goals of Kaleidoscope is that it be easyish for users, I suspect that when users start reaching for those sorts of keys, I would 100% take a pull request to add them to the library of available Key definitions.

Now, if you'd started this discussion with the definitions below these, I'd have been inclined to agree with you.

These items are all nutso and my gut feeling is that they have no business in the core:

define KEY_BACKLIGHT_DOWN 0xF1

define KEY_BACKLIGHT_UP 0xF2

define Key_BacklightDown (Key) { KEY_BACKLIGHT_DOWN, KEY_FLAGS }

define Key_BacklightUp (Key) { KEY_BACKLIGHT_UP, KEY_FLAGS }

define KEY_RIGHT_FN2 0xfe

define Key_RFN2 (Key) { KEY_RIGHT_FN2, KEY_FLAGS }

define KEY_LEFT_FN2 0xff

define Key_LFN2 (Key) { KEY_LEFT_FN2, KEY_FLAGS }

[malsyned]

If the intention of key_defs.h is to contain definitions for all printable characters so that Kaleidascope users can just pull in the ones they want then that makes plenty of sense to me -- perhaps then the actual issue is "key_defs.h doesn't contain nearly all of the printable symbols users might like to refer to"?

I would have complained about the other items you point at but, you see, I had no earthly idea what they were ;)

[malsyned]

A little background on how the current status quo was challenging for me as a prospective Kaleidoscope user:

I started with the question "how do I add new keys that type shifted scancodes?" and the answer wasn't exclusively "they're predefined and automatically included by the headers" and it wasn't "you create them yourself with the LSHIFT macro" -- it was "If they're already needed for Keyboardio's default layout, you just use them. Otherwise, you have to define them yourself."

See why I might want to suggest a change?

[algernon]

I think the most straightforward way forward is to add the missing and/or useful shifted codes to the headers. Either on-demand, when the need arises, or in one fell swoop. Perhaps it could be moved to a separate header, would one want different naming, and to cover the case where the host-side layout is not US QWERTY. (For example, US QWERTY Shift+4 is $, the same on Hungarian QWERTZ is !)

QMK does something similar with the keymap_extras directory of various extra symbols.