search

jasoncoon / esp8266-fastled-webserver

GNU General Public License v3.0
712 stars 360 forks source link

Combined active branches #211

Closed henrygab closed 2 years ago

henrygab commented 2 years ago

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!
jasoncoon commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

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 commented 2 years ago

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