@merlin is working on some interesting ideas to change key positions/addresses from two byte storage to one byte, in order to safe memory. When discussing with him, I was reminded about some related aspects that could be improved in Kaleidoscope.
In order not to forget them, I will summarize most of those below, together with ideas how to improve them.
Dear developers, please don’t feel offended by me writing this. You have done a great job so far.
What I state here are just ideas about how things could be optimized from the perspective of a long-time C++ programmer. None of the points I address below are related to any bugs/errors (hopefully). Most is about readability, type safety and portability. None of the improvements proposed here would affect neither performance, nor memory consumption. Only some changes would break existing code, but are still worth being considered carefully.
I believe that it is worthwhile optimizing things (if agreed on) as early as possible, before there are more and more users and devs around. Keyboardio is soon going to ship thousands of keyboards and then there will be hundreds of developers working with Kaleidoscope. The more people are going to use it, the more of them are affected by possible future interface changes. That’s why I believe it is a good idea to polish things as early as possible.
There are likely more improvement ideas apart from the ones stated below. Let’s collect and discuss them here!
Type Safety
Macros and Macro Functions
There are far too many macros and macro functions used in Kaleidoscope. None of them are type safe. I know that this is the way it is done in C, but C++ provides features that make things safer for the programmer, so why not use them.
All preprocessor macros, such as key definitions could actually be static constexpr constants
// in key_defs.h
constexpr ModHeldType CTRL_HELD = B00000001;
// instead of
#define CTRL_HELD B00000001
// ModHeldType would be uint8_t on most platforms (see below)
or
// in key_defs_keyboard.h
constexpr Key_ Key_A = { HID_KEYBOARD_A_AND_A, KEY_FLAGS };
// instead of
#define Key_A (Key) { HID_KEYBOARD_A_AND_A, KEY_FLAGS }
and macro functions could be inline constexpt functions to make things type safe. All that without performance loss.
inline constexpr Key LCTRL(const Key &k) { return Key(k.keyCode, k.flags | CTRL_HELD); }
This also would avoid compiler warnings (e.g. about narrowing conversions) and errors on other platforms (see portability). Of course, if this would be changed, we would have to stick with the old upper case macro names, to preserve compatibility.
Changing this is very easy, as it can mostly be done with regex based replacement in single files.
Apples and Oranges
Currently almost everything is uint8_t or uint16_t in Kaleidoscope. Without any performance loss and with a great gain in readability every type could be turned into a class, or at least into a typedef. Using classes (like Key) for such things prevents accidental assignment of information that is e.g. stored as uint8_t but of completely unrelated meaning (apples and oranges). This is also related to the portability aspects, discussed below.
This is harder to achieve than replacing macros, as it requires a careful analysis of the different types of data that is used throughout the whole firmware. Changes must be done with care.
Encapsulation
Packing and unpacking of information (juggling with bits) definitively belongs into a class. That’s nice about the Key class, but it is still far from perfect as it is abused to store/pack all sorts of information (keycodes and control information of different type). Here I would love to see a dedicated derived classes of Key_ that would code and decode information and would name what is actually stored. This would make things way more transparent.
There seem to be different ways, modifiers are coded in a Key_ object and in the keyboard HID report. For new contributers (like me), this is pretty confusing. Here dedicated classes definitely could improve readability.
That is a restricted amount of work as it means introducing proper derived classes of Key. Also it would require to change some functions from passing Key by value to passing if by (const) reference, for inheritance to work.
Portability
Also related to using dedicated types (classes or typedefs) instead of uint8_t. Doing so also simplifies porting Kaleidoscope to other architectures with other bitness. Some might be able to process e.g. uint16_t faster than uint8_t. It can sometimes pay off to waste memory (if there is enough - not on the atmega32u4, of course) to gain speed of execution. Classes like Key_ are a good way to allow different implementations of packed storage of data for different platforms.
Expense for changes, see Apples and Oranges above.
API Documentation
Currently all developers of new plugins have to browse the Kaleidoscope core that is distributed across files and (Arduino) libraries to find out what interface functions are available. This is tedious and scary, and thus possibly repelling for new devs.
What about using a tool like doxygen to document the plugin API? Or better, all of Kaleidoscope. The latter seems preferable as there is no well-defined plugin API, appart from plugin hook callback functions. Browsing plugin code, one can see that plugins are calling all sorts of functions they can access.
