FunctionalColor 2 16-color edition (development discussion)

(JD Lien) #101

Alright, I’ve fixed the namespace issue and updated the documentation to reflect that.

About the themeSelect() function, that was kind of a workaround because I couldn’t figure out how to allow the user to just pass the theme directly to the constructor… if I could do this, then a user could either specify one from kaleidoscope::FunctionalColors or they could specify their own and instantiate FC without having to use setColorLookup later. I think that would be better.

(By the way, I compiled with all but the default statement commented out from the themeSelect switch and it saved 1,476 bytes. So you’re right - that’s not insubstantial and consumes about 5% of the program storage space. I really ought to improve this.)

Otherwise I’m not too sure how I would approach fixing themeSelect. If I could, it might be nice to have it there just as a shorthand…

Could I create another function that actually executes setColorlookup so it doesn’t generate all of the colorLookup functions on compile?

(JD Lien) #102

What is the right way to check that this is set? assert(mainColorLookup); on the first line of setKeyLed? I tried this and uh… nothing breaks but I don’t fully understand what it does.

I’d think it should always be set because it’s not possible to instantiate FC in a way that wouldn’t run setColorLookup.

As for the lastFC, can I just set that on activate to ensure that it’ll be set, like so?

void kaleidoscope::LEDFunctionalColor::FCPlugin::onActivate(void) {
  this->refresh();
  lastFC = this;
}
(Noseglasses) #103

One solution could be to do without the second lookup if no mainColorLookup is defined.

void FCPlugin::setKeyLed(uint8_t r, uint8_t c) { 
  Key k = Layer.lookupOnActiveLayer(r, c);
  bool skip = false, none = false;
  auto color = exceptionsLookup(k, skip, none);
  // if there was no match, we continue to mainColorLookup
  if (none) { 
     if(!mainColorLookup) return;
     color = mainColorLookup(k, skip); 
  }
  if (skip) return;
  ::LEDControl.setCrgbAt(r, c, dim(color, brightnessSetting));
}// end setKeyLed()

The assert-macro is for debugging and not to be relied on in release-code.

Assertions are only active if the code is compiled without the NDEBUG macro being defined. When active the assert-macro checks its boolean condition. If the check fails, it outputs an error message then calls abort(). This is what it does on high-level platforms as x86.
On avr things are different. Without further modifications, your firmware will output nothing and enter an infinite loop if the assertion fails.

Assertions are useful to check that certain requirements are always met. I tend to add a lot of them during writing code, even when I am sure that I will write the code in a way that it is very likely that the assertion will always pass. Later on during maintenance and further development, it might happen that the assertions become violated. Then your original assertion message will help you to fix what broke. As assertions are supposed to vanish in release versions of the code, they don’t add any additional overhead. Additionally, they tend to improve code readability.

Be warned that in the current version of Kaleidoscope the -DNDEBUG is missing in the compiler command line. So currently assertions add an undesired additional overhead.

That will only work if the color mode is activated before the keyboard and thus the brightness-adjustment macros become active. I am not sure about this. You would need to check the firmware core to see when void kaleidoscope::LEDFunctionalColor::FCPlugin::onActivate(void) is actually called.

I suppose it is safest to just check the pointer on access.

...

FCPlugin* FCPlugin::lastFC = nullptr;
...
void FCPlugin::brightnessUp(uint8_t keyState) {
  if(!FCPlugin::lastFC) return;
  FCPlugin::lastFC->thisBrightnessUp(keyState);
}

void FCPlugin::brightnessDown(uint8_t keyState) {
  if(!FCPlugin::lastFC) return;
  FCPlugin::lastFC->thisBrightnessDown(keyState);
}
(Noseglasses) #104

The restricted resources of the atmega are the reason why I suggested the template based lookup function approach in the first place. Using this approach we take care that only those functions are compiled and instanciated that are actually used, thus not wasting any PROGMEM.

I would remove the themeSelect function as the problem of compile-and-link-only-what-you-use cannot be solved by a theme selection function. The only way to minimize PROGMEM usage is to let the user set lookup function pointers from within the sketch. This will cause only those templated lookup functions to be actually instantiated that are set and thus used. Maybe you could provide a macro like

#define FC_SET_COLOR_THEME(PLUGIN, COOLOR_MAP) \
   PLUGIN.setColorLookup(&kaleidoscope::LEDFunctionalColor::groupColorLookup<kaleidoscopoe::LEDFunctionalColor::COOLOR_MAP>);

If the user would like to switch between different themes at runtime, she/he could invoke FC_SET_COLOR_THEME from a keyboard macro like you do to modify brightness levels.

To keep backwards compatibility with the current interface, yet minimizing PROGMEM usage for those users that do not want to use themeSelect, you could move it to the plugin-header and make it a template method. This will cause themeSelect and all the lookup functions only to be compiled/linked when actually used from within the firmware sketch.

// Resides in Kaleidoscope-LEDEffect-FunctionalColor.h
//
template<typename IntType>
void FCPlugin::themeSelect(IntType themeID) {
    // Content unchanged
}

If you go this way, you could do without the selection enums and theme ID integers. If you don’t want to go this way, there is a minor issue with your use of theme selection enums. To ensure compatibility with future versions of the plugin, I would recommend to move those enums to the plugin header. It is just too easy/likely that during future maintenance by adding color themes the mapping of integers to themes might get screwed up and become out of sync with the enum values in the users’ sketches.

Besides, as it is kind of a C/C++ convention that only macro names are all caps, I would rather use camel-case for the enum names.

namespace kaleidoscope {
   
namespace LEDFunctionalColor {

      //User friendly list of themes included with FunctionalColor. These just map to 0-5.
      enum { ColorMap, MonoWhite, DuoColor, Colorful, DefaultColor };

      // For backwards compatibility
      enum { 
         COLORMAP = ColorMap, 
         MONOWHITE = MonoWhite, 
         DUOCOLOR = DuoColor,
         COLORFUL = Colorful, 
         DEFAULTCOLOR = DefaultColor 
     };
}
}
(JD Lien) #105

As always, thank you for the helpful information.

I have incorporated the checks to prevent errors from undefined values, so that’s out of the way.

I thought about what could be done with themeSelect for a bit. I really liked the idea of allowing users to easily choose from a selection of predefined themes without having to mess with anything in the setup… but really, the whole point of FunctionalColors is to give users full control over their colors, so this is probably not sufficient anyways for the kind of people who buy a highly specialized mechanical keyboard and then reprogram it themselves using a low-level language.

You did have a good idea to use a template function, but I figured I’d rather not have the additional complexity if it just adds significant overhead anyway, so I came to a compromise - I turned themeSelect into a very simple function that just assigns the default theme for users who want to get started by instantiating FunctionalColor with no arguments.

The annoying part is that I broke compatibility with all the constructors, but hopefully that doesn’t hurt too many people since it has only been out for 24 hours or so… I figured that if I’m making breaking changes, I might as well do it now.

So FC lost some functionality, but it’s probably well-worth it to improve its memory footprint by over a third. In addition, that macro for setting themes was a good idea, so I’ve gone ahead and implemented that, with the difference that I removed the namespace so that it could be used for custom themes. The documentation reflects that you’ll need to specify the namespace manually if you want to use builtin themes and aren’t using namespace kaleidoscope:LEDFunctionalColors.

I’ve already pushed the changes live, although I haven’t actually tested on real hardware yet as the changes are minor and I doubt anything will work differently. I wanted to make sure the documentation available reflects the new changes before anyone else starts trying to use this and I end up with frustrated people using this with outdated documentation!

(Noseglasses) #106

Where’s the additional complexity? The user doesn’t notice. The method is invoked the same way. The only change is to move it to the plugin header and make it a template. Why not let the user choose between comfort and PROGMEM?

(JD Lien) #107

Heh, mostly complexity for me in maintaining it! (But I know, it’s not really that complex other than the switch being tedious to edit if I add many more themes).

My thinking was that most people would want to save PROGMEM enough that it’d be worth it to add another line of code… at least then people wouldn’t unwittingly use a lot of extra memory by accident. What if, say, at some point FC ends up with 100 built-in themes? Wouldn’t that use too much memory at that point to justify?

I just commented out the themeSelect(int) function since I was thinking I might do something with it later, so I could add this back in at a later time, then create additional constructors that allow a themeID as a final parameter (after brightness) for those who want to use it.

Given the way the current constructors work after the last update, making this change shouldn’t break compatibility with existing implementations. I could release an update that allows an integer after the (optional) colorList and non-optional brightness which then invokes the template function for people who want a super easy way to switch between preset themes, with the caveat that their memory usage may increase significantly.

(JD Lien) #108

Alright, I’ve re-implemented the themeSelect(int) function in the way you proposed and now allow the user to specify a themeID using an int or a name in an enum inside the header file. I’ve pushed this version to git without documenting the changes (I’ll do this after a bit more testing).

I like this approach… the user can consciously specify a theme or easily switch between several built in themes, but there is only a memory penalty if they deliberately add a final int argument after specifying brightness. Works really well! :slight_smile:

(Noseglasses) #109

Non of FCPlugin's constructors are inlined (defined in the class definition or marked inline). Thus they will all be compiled. As the constructors that accept the theme ids are also compiled, themeSelect will unconditionally be compiled as well.

Have you actually tested this (see errors below)? If it works, the linker apparently eliminates all unused constructors (those that are not called from the sketch) and with them the themeSelect template method instance. Thus, only if the theme selection constructor is used, themeSelect will be linked.

However, I doubt that your code compiles. The reason is that you defined a non-template themeSelect method in the plugin header and a templated version in the implementation file. For the compiler those methods are completely unrelated. A template instance is assigned a symbol name that reflects the template method name and the template parameters used to generate the instance. Every template instance is, from the compiler’s point of view, an independent method.

See the following example (press Demangle to toggle C++ and mangled symbol names).

Therefore, it is likely that this causes a compiler error that informs you about the templated themeSelect method being undefined (in the class definition), while the non-template version of the method is lacking an implementation. The latter would cause a linker error once the (non-templated) themeSelect method is called from the sketch.

FWIW, kaleidoscope builder has the Make target decompile. Not sure how you build your firmware, though. make decompile does a disassembly of the final firmware binary. This can help you to find out what is actually linked into the firmware (grep/search the output for function/method names). Also, if that is of any use to you (not too many people can read assembler these days :wink:) you can check the assembler code. Even if you cannot read assembler language, you can at least see how changes in your C++ code affect the amount of (assembler) instructions in your functions. That can help to do at least some black box style optimization of firmware size.

(JD Lien) #110

Hmm, okay…

First, the code that was on GitHub at the time you wrote this definitely compiles in the Arduino IDE and runs properly on a Model 01. (I did have a pretty bad bug where one of the constructors wasn’t setting rgbLookupExc which I fixed right before going to bed, but that was before you wrote this).

I did try moving the constructors into the .h and inlining them, and that seems to work fine, however it actually ends up using 260 bytes MORE progmem than it otherwise did. (I have no idea how that works, but I’ve seen other weird behaviour, like how adding extra constructors seems to decrease memory usage).

I’m not sure if, with respect to themeSelect() you were talking about this:

  void themeSelect();
  template<typename IntType> void themeSelect(IntType themeID);

Is it not acceptable to overload themeSelect() in this manner? The first function just selects the default theme, but with a parameter, it selects any of them via a switch statement. I’ll change it to themeDefault() for clarity and to avoid confusion and potential errors in the future, but this has no effect on how anything runs.

This must not be compiling the entire switch statement, because using a constructor that uses the themeSelect(int) version uses about 3KB more RAM.

Perhaps I should rename themeSelect() anyways, just for clarity.

Since based on my quick demo, inlining constructors in the header increases memory usage, I will leave it as is for now unless you can provide me an explanation that I’m doing this horribly incorrectly…

Here’s the code I created in Kaleidoscope-LEDEffect-FunctionalColor.h (and removed from .cpp)

  inline FCPlugin(byte brightness, RGBLookupException rgbLookupExc)
   : exceptionsLookup(rgbLookupExc)
  {
    themeDefault();
    brightnessSetting = brightness;
  }

  inline FCPlugin(RGBLookupException rgbLookupExc, byte brightness = 210)
   : exceptionsLookup(rgbLookupExc)
  {
    themeDefault();
    brightnessSetting = brightness;
  }

  inline FCPlugin(byte brightness, RGBLookupException rgbLookupExc, int theme)
   : exceptionsLookup(rgbLookupExc)     
  {
    themeSelect(theme);
    brightnessSetting = brightness;
  }


  inline FCPlugin(RGBLookupException rgbLookupExc, byte brightness, int theme)
   : exceptionsLookup(rgbLookupExc)
  {
    themeSelect(theme);
    brightnessSetting = brightness;
  }


// Another constructor that uses the default theme  and allows an optional brightness.
  inline FCPlugin(byte brightness = 210){
  // Switch block to specify themeid
    themeDefault();
    brightnessSetting = brightness;
  }

// Another constructor that uses the default theme  and allows an optional brightness.
  inline FCPlugin(byte brightness, int theme){
  // Switch block to specify themeid
    themeSelect(theme);
    brightnessSetting = brightness;
  }
(JD Lien) #111

By the way, it turns out that the above code works unless you use themeSelect(int). If so - I now get an error undefined reference to void kaleidoscope::LEDFunctionalColor::FCPlugin::themeSelect<int>(int)'

(Noseglasses) #112

I’m sorry, my bad. I simply overlooked the templated definition in the header and got confused by the ambiguous naming. Probably I should code less and sleep more :blush: Now it’s obvious to me that your code compiles. Anyway, your suggested renaming of the non template version sounds reasonable.

However, I cannot find any explanation why the inlined constructors increase the amount of code. Here, checking the disassembly definitely might clarify things.

Something I still need to get used to, is the ability of the linker to eliminate unused code. In my daily life I spend a lot of time writing libraries where dead code elimination is not useful. You solution is smart. As long as the user does not rely on the integer/enum based theme selection, no additional PROGMEM will be wasted.

There is, however, still one minor drawback of your current approach with regard do the constructors. What if a user only wants to use a custom theme? The way constructors are implemented right now, either the lookup function for the default scheme is compiled or those used by themeSelect. What about having a constructor that does not initialize the main color lookup function (assigns nullptr)?

(Noseglasses) #113

That’s because the templated themeSelect is implemented in the .cpp file not the header. When the sketch is compiled, the compiler tries to find the template and instanciate it. As it cannot find it (it only sees the header) it skips the instanciation. Then finally, while the firmware binary is linked, the themeSelect<int>(int) symbol is missing which causes the error that you report. After you move the templated themeSelect's implementation to the header, the linker error is supposed to vanish.

(JD Lien) #114

Ah, that makes sense.

As for the issue you pointed out (always compiling in the default theme, even when it is not used), I was considering that before but wasn’t sure if there was an easy way to get around it.

I suppose that nothing should break now since setKeyLED() will just return if there’s no theme assigned… so I could make another constructor where if you specify null or false as a second (or third) parameter, it explicitly assigns no theme, which should save a few hundred bytes of memory.

I wish there was something more elegant for the user (ie the program automatically assigns the default theme if none is assigned in setup) but I have no idea how that could be done if it is possible at all.

(Noseglasses) #115

As far as I can see, that’s not possible. The reason is that for the default theme to be assigned, it needs to be compiled in the first place. Secondly, any method whose task it was to check if a theme is set and assign the default theme if not, would use the default scheme’s color lookup method. The linker would see this use and as a consequence would never eliminate the default lookup method. The default lookup method will then unconditionally be always linked.

(JD Lien) #116

I did a little testing with different versions of the .h and .cpp files. The original versions as currently on master, and one where all constructors are inlined in .h (inlineconstructors branch).

This is interesting and I don’t quite understand what is going on, but there is a slight difference one way or another by a hundred bytes or so in the memory usage when you aren’t using a constructor that calls selectTheme(). When you do call selectTheme, however, there is a pretty significant difference in the memory usage between the two versions - the inlineconstructors version saves about 2-3KB. I have tested this on the Model 01 and as far as I can tell, they behave identically.

Do you have any explanations on why this might be?

//Six different themes with themeSelect(), original:
Sketch uses 26098 bytes (91%) of program storage space. Maximum is 28672 bytes.
Global variables use 2028 bytes (79%) of dynamic memory, leaving 532 bytes for local variables. Maximum is 2560 bytes.

//Six different themes with themeSelect(), inline:
Sketch uses 24020 bytes (85%) of program storage space. Maximum is 28672 bytes.
Global variables use 2028 bytes (79%) of dynamic memory, leaving 532 bytes for local variables. Maximum is 2560 bytes.

 

//Six instances, no themeSelect(), original
Sketch uses 22024 bytes (76%) of program storage space. Maximum is 28672 bytes.
Global variables use 2028 bytes (79%) of dynamic memory, leaving 532 bytes for local variables. Maximum is 2560 bytes.

//Six instances, no themeSelect(), inline
Sketch uses 21904 bytes (76%) of program storage space. Maximum is 28672 bytes.
Global variables use 2028 bytes (79%) of dynamic memory, leaving 532 bytes for local variables. Maximum is 2560 bytes.

It appears that the inlined version is the way to go. To get it to work I did have to switch the order of where I declare the theme structs and rearrange a few things, but I got it working pretty well.

Update: I’ve added a few new features largely informed by your suggestions and have now merged them into master, which is the latest branch. This uses the inline constructors in the header and allows you to not use a theme.

(Noseglasses) #117

Without the dissassembly I can only guess.

Usually, the difference between code size of inlined versions and non-inlined ones becomes greater the more often the inlined functions are invoked. As inlining means that the function body just replaces the call, inlining normally tends to increase binary sizes. Only if an inlined function is very short and used only once, the inlined binary might be slightly shorter as no function instance is generated and the code for stack handling and passing parameters/return value can be safed.

The size differences between with and without themeSelect() makes sense but the rest of the results seems really weird, however very interesting.

If you want me to have a closer look, you could create four gists of the disassemblies and post them.