FunctionalColor 2 16-color edition (development discussion)

[merlin (Michael Richters)]

Look in HIDTables.h for the numerical values.

[noseglasses]

@jdlien

Is it safe to assume that all keys defined consecutively (and that should reasonably stay that way through future revisions of Kaleidoscope) would have consecutive keycodes? I presume these are all given by the compiler and not under the direct control of a developer…

The actual HID standard bytes are in KeyboardioHID/src/HIDTables.h those are taken from the USB HID protocol standard which is not likely to change soon.

@jdlien

That being the case, I could use >= && <= for letters, numbers, punctuation (Key_LeftBracket through Key_Slash), F1-F12, arrows, keypad keys, and maybe even modifiers (LeftControl through RightGui).

Exactly.

@jdlien

In addition, I’d want to allow customizing the consumerctl keys and have noted that some of your macros seem to presume that keys all start with Key_ which isn’t the case here… but I presume this can be rectified by just making the user enter Key_A instead of simply A, and leave that out of the macro. (Unless there’s a better way).

Good point. My macro was defined for lazy users but your idea of generalization makes sense to me.

@jdlien

It looks like all the Consumer keys could be grouped very easily in this way as well… (which is handy as there is a nutty number of them).

Yep.

[noseglasses]

I forgot to mention that I would recommend against using the macros defined in KeyboardioHID/src/HIDTables.h directly in your code.

[jdlien]

Something I’m struggling with a bit in implementing your suggestion is how to call a function from a class within that class’s definition. Specifically, in the .h file, in the macro, if I wanted to use my example “isNumber()” function, or even a dim() function, I get an error that it isn’t declared in this scope.

#define FC_CB_END_COLOR_LIST \
   } /*end switch*/ \
    /*Handle colors for group members without specific colors here */ \
    if(isNumber(k)) {return numberCB;} \
   static byte default_cb = (defaultPaletteId << 4) | defaultBrightness; \
   return default_cb; \
}

Noob question, I know, but how can I handle this?

[jdlien]

I’m getting closer…

kaleidoscope::LEDFunctionalColorCB::isNumber(k)

But now I need the object name in order to use the function, so I’ve got to figure out how to reference that.

[noseglasses]

If you are using the FC_CB_START_COLOR_LIST/FC_CB_END_COLOR_LIST macros in the sketch, they are defined at global scope and the function is a global function, where a class member is not available unless a public static member.

You say you do it in the class definition? Something like

class FC {
   public:
      FC_CB_START_COLOR_LIST
      ...
      FC_CB_END_COLOR_LIST
      ...
};

This must be

class FC {
   public:
      static FC_CB_START_COLOR_LIST
      ...
      FC_CB_END_COLOR_LIST
      ...
      static bool isNumber(...) {...}
};

Else you would define a non-static member function. Only static member functions can be used in the same way as pointers to global functions (There are also pointers to member-functions but that is a completely different matter and of no use here).

[jdlien]

Of course! Those functions make a lot more sense as static member functions since they don’t need to refer to anything specific about the object. I managed to get that working… but the way I had intended to implement it was to have member variables for each instance of the class to record the colorbyte associated with each group… (numberCB in the example I posted earlier).

I suspect this approach won’t work really well from within the macros, however, since there’s no instance yet, and thus I can’t read it.

Would it make sense to make these variables static as well? I suppose that would prevent you from having multiple instances with distinct settings for the colorgroups…

Otherwise I’d have to define them all through arguments to the macro… which just sounds really complicated for the end user to me. Any ideas?

[noseglasses]

@jdlien

Would it make sense to make these variables static as well? I suppose that would prevent you from having multiple instances with distinct settings for the colorgroups…

Not really. Do you want the colors to be fixed or runtime changeable?

[noseglasses]

// Defined by your plugin. Either static class member or global in plugin namespace
//
template<typename ColorMap>
cRGB specificColorLookup(const Key &k) {
   if(someCondition(k)) {
      return ColorMap::color1;
   }
   ...
}
...
// In the sketch
// in namespace/global scope
//
struct MyColorMap  {
   static constexpr cRGB color1 = green;
   static constexpr cRGB color2 = red;
   ...
}
...
// In setup()
//
myPluginInstance.setColorLookup(specificColorLookup<MyColorMap>);

This is a perfect usecase for template functions.

[jdlien]

So… the setColorLookup() function is being passed a reference to the specificColorLookup() function here? Why do it this way, instead of just passing a reference to MyColorMap instead, then using that to override the defaults in a structure within the class?

I presume there’s some kind of efficiency benefit to your approach that I’m not appreciating.

[noseglasses]

MyColorMap is a type that has only compile time constant members and is never instanciated. Sounds strange, no? The templated function uses it like a user definable palette with named entries. With this approach all colors are compile time constant yet user definable. Any number of specific color lookup functions can be defined by your plugin. The functions define which names must be present in ColorMap. Templates are a bit like duck typing.

[noseglasses]

Imagine a specific color lookup function template<typename ColorMap> cRGB colorModifiers(const Key &k).
ColorMap would then be required to define e.g. colorShift, colorAlt and colorCtrl.

[jdlien]

I’m kind of stuck on how I would create the setColorLookup() function. I think I at least understand how to create a function that accepts another function as a parameter by reference, but after that, what do I do with it?

template<typename ColorMap>
cRGB groupColorLookup(const Key &k) {
   if(LEDFunctionalColorCB::isNumber(k)) {return ColorMap::numberColor;}
}

void setColorLookup(cRGB (&gcl)(const Key)) {
  gcl(???); //how to specify a key as the parameter?
 // What else goes here?
}

[noseglasses]

It seems I managed to confuse you Just take the existing LEDFunctionalColorRGB class and modify it slightly. Remove the now obsolete setKeyColor method.

class LEDFunctionalColorRGB {
   public:
      typedef cRGB (*RGBLookup)(const Key &);
      void setColorLookup(RGBLookup rgbLookup) {
         rgbLookup_ = rgbLookup;
      }
      // No further changes required
      ...
};

I would suggest to move functions like isNumber to an extra header in a suitable namespace and make them inline constexpr at global scope. That header might then be a candidated to be submitted to the firmware core repo later.

[jdlien]

You’ve had me confused for days but it doesn’t take much. Today I spent several hours trying to figure out why I couldn’t compile until I realized that I needed a semicolon after the colormap struct definition. (That should have been a lot more obvious than it was but somehow I thought the problem was in my header file. facepalm)

What I’m currently stuck on now is the call to setColorLookup. I’m getting an error: “error: expected primary-expression before ‘groupColorLookup’”
on this line:

fcRGB.setColorLookup(kaleidoscope::LEDFunctionalColorRGB groupColorLookup<FColorMap>);

Edit:
I might not have specified the scope correctly. After changing it to

fcRGB.setColorLookup(kaleidoscope::LEDFunctionalColorRGB::groupColorLookup<FColorMap>);

I now get the error

error: no matching function for call to 'kaleidoscope::LEDFunctionalColorRGB::setColorLookup(<unresolved overloaded function type>)' 

[noseglasses]

I need the full code to maybe tell you what goes wrong. Can you push it to a branch of your github repo?

[jdlien]

It’s actually the master branch of my repo right now. I’ve made a pretty big mess of that.
I have added the busted .ino file to the examples directory alongside the one you submitted in your PR.
Hopefully there’s something simple wrong there that you can quickly point out. I’d say it’s far more likely that I’ve just made a giant mess, though. I’m a bit in over my head!

jdlien/Kaleidoscope-LEDEffect-FunctionalColor

[noseglasses]

Templates are different in that the whole implementation must be visible at the time of instancitation.

Move the implementation of groupColorLookup to the plugin header and you should be fine.

[jdlien]

Okay, good to know… but I’m afraid that didn’t get me any farther through compilation. I’m still getting the same error with the implementation in the header.

Kaleidoscope-LEDEffect-FunctionalColor.h:161:36: error: extra qualification 'kaleidoscope::LEDFunctionalColorRGB::' on member 'groupColorLookup' [-fpermissive]
   template<typename ColorMap> cRGB LEDFunctionalColorRGB::groupColorLookup(const Key &k) {
                                    ^
/Users/jdlien/Dropbox/Keyboards/Keyboardio/Model01-Firmware_noseglasses/Model01-Firmware_noseglasses.ino: In function 'void setup()':
Model01-Firmware_noseglasses:401: error: no matching function for call to 'kaleidoscope::LEDFunctionalColorRGB::setColorLookup(<unresolved overloaded function type>)'
  fcRGB.setColorLookup(kaleidoscope::LEDFunctionalColorRGB::groupColorLookup<FColorMap>);

[noseglasses]

The first error should vanish if you remove theLEDFunctionalColorRGB in line 161 of the plugin header.

Also modify

typedef cRGB & (*RGBLookup)(const Key &);

to

typedef cRGB (*RGBLookup)(const Key &);

then it might work.

Sorry, I’m currently on a rock climbing trip in Italy and only have my smartphone with me. So I can’t compile your code.

Keep me updated!