Initial implementation for OLKB Planck keyboard #459
Conversation
Signed-off-by: Jesse Vincent <jesse@keyboard.io>
Signed-off-by: Jesse Vincent <jesse@keyboard.io>
There was a problem hiding this comment.
I like where this is going, and the implementation looks sweet on the first read. I left a few in-line suggestions, which I think should be corrected before merging.
There are a few places where there are more than one empty line in one row, those should be collapsed to only one empty line, I think. I didn't mark every place, just one or two - these are small details though. The bigger ones are in-line.
| uint8_t matrix_row_pins[] = { PIN_D0, PIN_D5, PIN_B5, PIN_B6 }; | ||
| uint8_t matrix_col_pins[] = { PIN_F1, PIN_F0, PIN_B0, PIN_C7, PIN_F4, PIN_F5, PIN_F6, PIN_F7, PIN_D4, PIN_D6, PIN_B4, PIN_D7 }; | ||
| uint8_t matrix_columns = sizeof(matrix_col_pins); | ||
| uint8_t matrix_rows = sizeof(matrix_row_pins); |
There was a problem hiding this comment.
I'm not sure about these ones, in the sense that I'm not convinced that having them unde the kaleidoscope::hardware::olkb namespace is sufficient. I have this - perhaps unfounded - fear that once there's another port that uses the same namespace, the symbols would conflict. Granted, they are local to this file, but they aren't static - thus, they are accessible elsewhere. Perhaps moving them to a k::h::o::planck namespace would be better? Or marking them static. Or both.
(Mind you, I haven't tried if these cause issues, its just a hunch).
| uint8_t matrix_rows = sizeof(matrix_row_pins); | ||
|
|
||
| uint16_t Planck::previousKeyState_[matrix_rows] = {0} ; | ||
| uint16_t Planck::keyState_[matrix_rows] = {0}; |
There was a problem hiding this comment.
As both of these are static, they are guaranteed to be initialized to zero, and the extra initialization is not needed. We could save a few bytes by not initing them explicitly, I reckon.
There was a problem hiding this comment.
No change to compiled size, but happy to kill it.
|
There's one more thing I'd note: there is no README for the new hardware plugin. Could you add one? To describe which variants of the planck are supported, and which features of it? IIRC there are non-avr plancks out there now, and they all support sound, something this port does not. No need to be verbose, just some pointers and hints. |
Signed-off-by: Jesse Vincent <jesse@keyboard.io>
@algernon, could you please review this implementation of a new hardware port and, if it looks ok, merge it?