LCD menu code refactoring and cleanup

[marcio-ao]

I was trying to understand the macros in "menu.cpp" and "menu.h" and I found that the code was very hard to follow due to the fact method names were concatenated using macros. This is an attempt to simplify things a bit.

• Related menu item edit methods are now grouped into a class.
• Eliminated repetition of definitions in macro arguments.
• Increased legibility by reducing the use of ## operators.

[marcio-ao]

What is this for?

[thinkyhead]

To enforce the use of a semicolon at the end of the macro during usage. If the macro wasn't meant to be used outside of method / function context then it would be possible to just use (0) which is a legal empty C++ statement. Unused typedef also has the advantage of not throwing a compile warning, which an unused global variable would do.

[marcio-ao]

I think this isn't needed, but I left it in.

[thinkyhead]

bool _edit() was originally used as the common function for two primary edit items — one without a callback and one with a callback. The one without a callback ignored the returned bool. The one with a callback used the returned bool to decide whether or not to invoke the callback. Later changes moved the position of the callback to fix a bug, so the returned bool is now obsolete and that method can be discarded.

[marcio-ao]

What is this for?

[thinkyhead]

This is good stuff! Does it have any positive effect on the binary size?

One change that would probably reduce the binary size a little would be to replace this:

template<typename NAME>
void menu_item_template<NAME>::action_setting_edit(PGM_P const pstr, type_t * const ptr, const type_t minValue, const type_t maxValue, const screenFunc_t callback/*=NULL*/, const bool live/*=false*/) {
  lcd_save_previous_screen();
  lcd_refresh();

  editLabel = pstr;
  editValue = ptr;
  minEditValue = minValue * scale;
  maxEditValue = maxValue * scale - minEditValue;
  encoderPosition = (*ptr) * scale - minEditValue;
  currentScreen = edit;
  callbackFunc = callback;
  liveEdit = live;
}

…with a call to a handy helper function, like so…

// A single helper function to init item editing for all types
void init_item_editing(PGM_P const el, void * const ev, const int32_t minv, const int32_t maxv, const uint32_t ep, const screenFunc_t cs, const screenFunc_t cb, const bool le) {
  lcd_save_previous_screen();
  lcd_refresh();
  editLabel = el;
  editValue = ev;
  minEditValue = minv;
  maxEditValue = maxv;
  encoderPosition = ep;
  currentScreen = cs;
  callbackFunc = cb;
  liveEdit = le;
}

template<typename NAME>
void menu_item_template<NAME>::action_setting_edit(PGM_P const pstr, type_t * const ptr, const type_t minValue, const type_t maxValue, const screenFunc_t callback/*=NULL*/, const bool live/*=false*/) {
  const int32_t minv = minValue * scale;
  init_item_editing(pstr, ptr, minv, int32_t(maxValue * scale) - minv, int32_t((*ptr) * scale) - minv, edit, callback, live);
}

[thinkyhead]

First test, a CR-10S with lots of features. The build size has increased by 1224 bytes. Not good.

[thinkyhead]

Hmm… My suggested optimization doesn't do much. It saved only 18 bytes in my test.

[thinkyhead]

Go figure. Tweaking menu_item_submenu recovered 1052 bytes. Other changes, only a little. If we can recover 90 more bytes, everything will be even-steven as before this PR.

[thinkyhead]

Hmm… My last test shows this PR costing 1096 bytes more than the old code. We need to do better.

[thinkyhead]

I hope this isn't all just Arduino 1.8.5 being weird. But, with the last commit the size is actually 66 bytes smaller than the code before this PR. We'll have to re-confirm with more configurations and different compilers.

[thinkyhead]

Strange build results… Using PlatformIO to build…

169936 / 5048 : Old menu code.
170556 / 5048 : After refactor (+620)

Using Arduino IDE:

168012 / 5042 : Old menu code
167946 / 5042 : After refactor (-66)

I tried to import the Arduino compile flags to PlatformIO, but saw no difference.

So, it's weird.

[GMagician]

I also saw something like that some time ago with a PR of mine...
It was weird then as it is now. The miracle of compiler optimization

[marcio-ao]

Hmm… My last test shows this PR costing 1096 bytes more than the old code. We need to do better.

It depends on the compiler in use, I suppose. Using avr-gcc via the Makefile, I see a minor improvement:

Before: 186342 bytes (71.1% Full)
After: 185726 bytes (70.8% Full)

For the record, my objective was making the code easier for me to read, without necessarily changing any of the generated code. It is mostly renaming and reorganizing methods and converting macros into templates and compile-time expressions. Any change in code size is probably due to compiler idiosyncrasies.

I suspect it may in fact be possible to make the Ultralcd code smaller, since there is a lot of repetition in he generated code, but this was just my first pass at cleaning things up.

[marcio-ao]

void init_item_editing(PGM_P const el, void * const ev, const int32_t minv, const int32_t maxv, const uint32_t ep, const screenFunc_t cs, const screenFunc_t cb, const bool le)

@thinkyhead: I see what you are getting at. The key observation you made is that not all methods in the template vary with the template parameter. If we can get the compiler not to specialize different copies of that method, we will save space.

There is actually a more C++ way of solving that problem that does not involve a helper method. That is simply be using template member functions on a non-template class. I will experiment with that and see if I can get more code size reduction out of it.

[marcio-ao]

With my latest commit, I got the "avr-gcc" build down even further:

Before: 186342 bytes (71.1% Full)
Commit c7bef82: 185726 bytes (70.8% Full)
Commit c10357d:  183950 bytes (70.2% Full)

This values are a for our FW, which has different things enabled than the default, so you should look at the change, rather than the absolute values. I am seeing that each commit made things smaller.

Curious to see whether it helps for the other build platforms too.

[marcio-ao]

These are the results I see when compiling our FW using Arduino IDE 1.8.5:

Before: Sketch uses 175856 bytes (69%) of program storage space. Maximum is 253952 bytes.
Commit c7bef82: Sketch uses 173902 bytes (68%) of program storage space. Maximum is 253952 bytes.
Commit c10357d: Sketch uses 173604 bytes (68%) of program storage space. Maximum is 253952 bytes.

So I am seeing a drop with the very latest commit.

[marcio-ao]

@thinkyhead: I think I am done here. I can't think of any other ways to massage this code either to make it more readable, or to make it take up less space.

With "Makefile/avr-gcc", I am seeing the following code size reduction:

Before PR: 186342/5496 bytes
After PR: 183732/5496 bytes  (-2610 bytes)

With Arduino IDE 1.8.5:

Before PR: 175856/5466 bytes
After PR: 173640/5466 bytes  (-2216 bytes)

I don't use PlatformIO, so I don't know what kind of results we might get there, but hopefully something comparable.

[marcio-ao]

@thinkyhead: What was the code size difference in PlatformIO after my final commit?

[thinkyhead]

For my AVR build (loaded CR-10S) config, reverting this PR gives me…

DATA:    [======    ]  61.1% (used 5002 bytes from 8192 bytes)
PROGRAM: [=======   ]  66.6% (used 169066 bytes from 253952 bytes)

And keeping this PR gives me…

DATA:    [======    ]  61.1% (used 5002 bytes from 8192 bytes)
PROGRAM: [=======   ]  65.9% (used 167310 bytes from 253952 bytes)

So that's pretty impressive. This PR is now reducing the binary size by 1756 bytes.

[marcio-ao]

So that's pretty impressive. This PR is now reducing the binary size by 1756 bytes.

SWEET!