Combined active branches

[henrygab]

Summary

This is the combined set of changes from the following branches...

• combined_dev
• combined_dev2
• combined_dev3
• combined_dev4
• combined_dev5
• combined_dev6
• combined_dev7

---

Recommended method of review

• Review the high-level description of the changes below

• Review the changes to files under include/config and include/controllers below,
  and mark the "Viewed" checkboxes for those first.

• Switch over to the "Files Changed" tab

• Start the review with those files having a single comment that starts with "SIMPLE TO REVIEW",

  • Use the "viewed" checkbox to track files you've completed review of

• When done with the "SIMPLE TO REVIEW" files, you should have only four files left for review.

• These files may have multiple comments
  inline with the corresponding code, to guide the review.

Of course, open your own review with questions/comments for any file:

• click on the first line of code
• optionally shift-click on the last line of code
• click the blue "+" symbol that appears to start the comment
• choose "add review comment" to start a multi-comment review
• When done with all files, click the green "finish your review" to post
  all your pending comments / questions.

In this way, you need not do all the review in a single sitting. The "SIMPLE TO REVIEW" might
be easy enough to do first, for example.

---

High-level Description of Changes

Goals of this PR included

• Merge the code from the "active branches" into main
  • 1628-rings
  • All of the fibonacci* branches
  • kraken64
  • parallel
• Maintain compatibility with Arduino as the build environment
• Remove restriction on git clone directory

This PR meets the above goals, enabling to change which "product" is built by simply
uncommenting the corresponding line in the file "config.h". Analysis of the active
branches discovered many variations between the branches, but also much commonality.

The variations are generally limited to the following files

• config.h -- defines which product to build
• include/configs/product/*.h -- defines the capabilities, pin overrides, product names, etc.
• map.cpp -- For products with defined pixel positions (HAS_COORDINATE_MAP),
  this file defines those pixel positions in a common format.

---

Highlights of the changes include:

• Moved all source files to subdirectory. This fixed a problem where Arduino
  would fail if the depot was cloned to a directory with any name other than
  the original. Moving to a subdirectory allows client to have multiple
  clones of the depot, with any directory naming they wish. As a free bonus,
  it also simplifies the eventual use of PlatformIO.
• Converted files that were incorrectly named as headers to be correctly
  marked as C++ sources. Generated corresponding header files as needed.
• Fixed various bugs discovered during the merging of the active
  branches, as previously mentioned in the discussions.
• Changed references to quantites of "LEDs" to instead refer to quantities
  of "pixels". After all, a single RGB pixel is composed of three LEDs
  (one red, one green, one blue). Although less common, other pixels types
  exist with four or more LEDs integrated into a single pixel (e.g., RGBW).
  This change reduces confusion by defining things as per-pixel instead,
  such as the maximum power draw per pixel.
• HTML files now dynamically update the product name.

---

Hope this helps!

[henrygab]

This review covers those files that fit in the "SIMPLE FILE REVIEW" bucket.

Only two files will remain for review after these are reviewed:

• map.cpp -- which is slightly more complex because it supports all the variability for most of the products
• esp8266-fastled-webserver.ino -- just because it's a large file

[henrygab]

This covers my comments for map.cpp and the main .ino file, which are larger files.

[jasoncoon]

Thank you, this is seriously amazing! I tested this briefly last night, and hope to spend more time testing each different config/model/product tonight. But quick tests on the default and F256 worked great, and switching between them was extremely easy.

I did all of my testing in the Arduino IDE, and only had to change subdirectory includes to use forward slashes (on MacOS) instead of back slashes. Hopefully forward slashes also work on Windows. I found a few other small things I'd like to change, but nothing that would prevent this from being merged. I'll comment on them inline later.

I understand the reasoning on the change from NUM_LEDS to NUM_PIXELS, and only used NUM_LEDS because all of the FastLED documentation and examples use it. I'm fine with keeping NUM_PIXELS.

What would you think, after these changes are merged, about splitting up the Map.cpp file, and putting the maps in the corresponding product header files?

[henrygab]

Thank you, this is seriously amazing! I tested this briefly last night, and hope to spend more time testing each different config/model/product tonight. But quick tests on the default and F256 worked great, and switching between them was extremely easy.

Wow, I'm humbled by your response, yet thrilled to hear that quick tests worked well.

Hopefully forward slashes also work on Windows.

I've just pushed a fix for the include paths... I totally missed that. Yes, "proper" path separators work when compiling under Windows. 🙂

I found a few other small things I'd like to change, but nothing that would prevent this from being merged. I'll comment on them inline later.

Great! Looking forward to reading those!

What would you think, after these changes are merged, about splitting up the Map.cpp file, and putting the maps in the corresponding product header files?

Good question. While the structures currently in map.cpp can be declared in a header file, they should only be instantiated in a single .cpp file. This doesn't matter so much for Arduino, because Arduino just concatenates all the source files into one file during build (yes, really). But, it's not recommended for many other reasons.

HOWEVER, I definitely want to do something in that area. The fact that there are so many product-specific bits in map.cpp breaks the intended goal of "keep all the per-product variable stuff together" goal. As you can see, you hit right on the part I was least pleased with. 🤣

My best thoughts thus far (skipping the many terrible solutions I've discarded):

1. Create per-product source files (e.g., ./include/configs/products/kraken64.cpp)
2. Move the corresponding data declarations from map.cpp into those files
3. In config.h, have a conditional include of those data files:

// Product-specific configuration
#if defined(PRODUCT_DEFAULT)
    #include "./include/configs/product/default.h"
#elif defined(PRODUCT_1628_RINGS)
    #include "./include/configs/product/1628rings.h"
    #if INSTANTIATE_GLOBAL_DATA // define this symbol in exactly ONE file
        #include "./include/configs/product/1628rings.cpp"
    #endif
#elif defined(PRODUCT_KRAKEN64)
    #include "./include/configs/product/kraken64.h"
    #if INSTANTIATE_GLOBAL_DATA // define this symbol in exactly ONE file
        #include "./include/configs/product/kraken64.cpp"
    #endif
#elif // ... repeat as needed

What do you think of that hack?

(avoids preprocessor line length limit, macro requirement to be single line, ... so it's the least terrible idea I have thus far.)

[henrygab]

Ah, there were two things that I wanted to ask about....

EEPROM

Saving of setting to EEPROM.... writeAndCommitSettings() was previously called every time the user made any change, and anytime the IR remote changed any setting. However, I thought EEPROM had a very limited number of times it could be written?

Also, at least on ESP8266 and ESP32 boards, there's a built-in flash file system. In fact, that's where the HTML source files go. Thus....

Other than getting something working quickly, was there any other reason to use EEPROM instead of the file system for settings?

EDIT: Turns out that the ESP8266 and ESP32 only emulate an EEPROM; The ESP8266 uses a portion of the non-volatile storage; the ESP32 creates a file (or so it says on ArduinoJson.org). Well... that's really good news, actually. 🙂

JSON

It appears that you've hand-coded some JSON generation. ArduinoJson (repo) makes JSON reading/parsing/generation easy ... almost too easy.

Any objection to adding a dependency to that library?

[tobi01001]

I do believe that it is still good to limit write cycles to the flash.
Writing the data to the EEPROM with every change (or even call to the rest / IR interface) might still be quite a lot in terms of write cycles. I would - for these not use the LittleFS as the overhead seems to be too much?
For myself I did the following:

• cyclic check of any modified settings (currently 100 ms)
• broadcast the changed ones via websocket (using ArduinoJSON)
• set a flag to save them sometimes to EEPROM
• cyclic check if we need to write the EEPROM data (currently 5 seconds)
  I did add a CRC to the EEPROM data to do a sanity check at "boot" and reset all to default values if there is a CRC mismatch (instead of a magic number as I once created a bootloop as not all code is written that defensive).
  All settings are within a structrue that gets completely written to the EEPROM so one does not have to care about a settigns size or position...

I do use ArduinoJSON (and would also recommend that) for data exchange with the server - client communication as it makes that handling pretty easy.

I did not think this one through yet but one could also use ArduinoJSON for the field and fields array to ease and unify the handling.

...and I do have a single function to modify all settings (which I recently modified to just make use of the Fields array). This is just making the maintenance easier as a new or modified parameter will not require a new "webserver.on" callback.

Just my few thoughts / ideas to further improve...

Regards,
Toby

[henrygab]

Perhaps I should now focus on PlatformIO. It'll make debug / build / etc. much easier. Let me know if any issues with this PR.