With KeyAddr: foo(KeyAddr(2, 3)), or foo(key_address) if we got the address ourselves from someplace else.
With indexes: foo(19) or foo(key_address). In this case we need helpers to convert to- and from between coordinates and index.
With index + helpers: foo(KeyIndex(2, 3)) or foo(key_address).
In all three cases, the current syntax would still work, because we’ll make sure to have backward compatibility.
So the look of the code is pretty much exactly the same. But in the index + helpers case, the code that implements this is much, much simpler. And that simplicity is what I campaign for.
There are some subtle differences, like when iterating, we currently use COLS * ROWS as maximum, while with KeyAddr… I’m not sure what we’d use there, but we can provide a constant for that too, without having to use a complicated class.
From the user point of view, there is very little difference between what @noseglasses proposed, and mine. The one major difference I can see is that with KeyAddr, you can do both foo(KeyAddr(1, 1)) and foo(KeyAddr(9)), and both mean the same thing (assuming a 8-column keyboard, and an index = row * 8 + col transformation). With mine, you’d write this as foo(KeyIndex(1, 1)) and foo(9). In both cases, foo(1, 1) would still work.
The major difference is that his code provides more info to the compiler, which can then use that information to error out or warn about wrong use. That’s a good thing, but not worth the complexity, in my opinion.
…whereas, with a simple typedef, we can skip all that documentation, because we already know how to deal with an integer. And if a plugin implementer doesn’t understand that he or she shouldn’t use key addresses that don’t exist, well, it’s not likely to be a useful plugin.
It’s also a bad thing. I’d say about 50% of the time I’ve spent working on Kaleidoscope code has been struggling to get it to compile. Fixing bugs in code that compiles is – for me, anyway – much, much easier.
This is one of the reasons I wanted to change to a single-number index for keys. I created TOTAL_KEYS for this purpose, replacing the need for ROWS & COLS outside of Kaleidoscope-Hardware-* classes.
The one thing I think rows and columns are still useful for is user-identifying the keys on the keyboard. Though, on the Model01, that’s not really clear what it should be, since it’s not a simple grid.
We could, but that makes it much harder to write some of the existing LEDEffect plugins, like Chase and Rainbow. I’ve tried replacing setCrgbAt() and I get clearly not what was desired from several LED plugins.
It’s worth pointing out that I also changed the key indexes in my experiment, to see how much more efficient I could make the code; the column numbers are all reversed. I don’t know if those gains would be worth it, but it did certainly make the hardware module simpler (and there are a number of additional simplifications possible because of that).
No, it doesn’t. The only reason for it to convert rows and columns is to make existing code backwards compatible. If we make this change, the only rows and columns that a plugin would need would be ones of their own devising.
So that operator StorageType() { ... } translates to operator uint8_t() { ... } (or whatever KALEIDOSCOPE_KEY_ADDR_STORAGE_TYPE is defined to). Search for C++ cast operator instead!
This appears to be a reasonable guide about the topic of typecast operator overloading, with examples and all that. Starting from a getter/setter setup, towards a cast overload.
For the M01, KALEIDOSCOPE_KEY_ADDR_STORAGE_TYPE would be uint8_t.
A cast operator has an unusual syntax, as the return type is part of the method name. The cast operator method is, at the point of definition, named operator StorageType() but could also be named operator uint8_t().
It can be used as
KeyAddr a(1, 2);
// Standard use
KeyAddr::StorageType t1 = a;
// Explicitly
auto t2 = static_cast<KeyAddr::StorageType>(a);
// (pretty unconventional, just for completeness)
auto t3 = a.operator KeyAddr::StorageType();
// Pretty unconventional and less portable (just for completeness)
auto t4 = a.operator uint8_t();
Funny, isnt’t. A method that has a polymorphic name.
I just added the weird ways to call it to show what C++ does under the hood. Please, don’t get this wrong, no-one would actually write
and that’s what the cast operator is good for. To enable a class to pretend to be another type e.g. in assignments or when passed as a function parameter, without requiring the user to add an explicit type conversion.
A typedef referring to a #defined alias for a built-in type, then used as the type specifier for a cast operator, which has no syntactic salt helping to explain what has just been declared.
That’s several layers of type indirection, and I can completely see how someone might miss the typedef and think that StorageType is something built-in to C++, but somehow doesn’t show up in any of their searches.
That would be very frustrating and off-putting.
Would things maybe have been better if instead of the very generic term StorageType something more specific had been used like KeyAddrInternalType, something that couldn’t reasonably be mistaken for language-level magic?