Single-byte key addressing (for boards with fewer than 256 keys)


(Gergely Nagy) #41
  • Current code: foo(2, 3) or foo(row, col).
  • 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.


(Michael Richters) #42

…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.


(Michael Richters) #43

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.


(Michael Richters) #44

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.


(Gergely Nagy) #45

…and it’s not a simple grid on the Shortcut either, which has holes in the matrix.


(Michael Richters) #46

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.


(Michael Richters) #47

My other concern exactly!


(Noseglasses) #48

Wow, I was AFK for an hour and so many posts…

Strange. I thought it was you who suggested overloading a cast operator…

…that’s why I did it. I thought you were interested in seeing how it is done.

operator StorageType() {...}

is the cast operator. It casts a KeyAddr to an integer of type StorageType.


(Noseglasses) #49

Not exactly, it is a bit more than an integer, it converts and packs rows and cols.

Hm… that’s what people call type safety.


(Michael Richters) #50

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).


(Michael Richters) #51

Please explain further. I’m coming up empty on Google searches for “C++ operator StorageType”.


(Noseglasses) #52

No you won’t. What about fromCoordinates(...).


(Michael Richters) #53

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.


(Noseglasses) #54

Sure, the constant would be of type KeyAddr.

constexpr KeyAddr cMaxKeyAddr(COLS, ROWS);

(Gergely Nagy) #55

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!


(Michael Richters) #56

Oops. In my haste to try to keep up with the discussion, I failed to even finish the paragraph. Sorry.


(Michael Richters) #57

I did. Everything I’ve found so far, including the code here is completely opaque to me. I can’t follow any of this now, I’m so confused.


(Gergely Nagy) #58

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.


(Noseglasses) #59

Check this out:

In my example StorageType storage type is a typedef of an integer in the KeyAddr class. Its use is to enable portability.

 typedef KALEIDOSCOPE_KEY_ADDR_STORAGE_TYPE StorageType;

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

auto t2 = static_cast<KeyAddr::StorageType>(a);

everybody would call

KeyAddr::StorageType t1 = a;
uint8_t t2 = a; // less portable

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.


#60

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?