search

jasoncoon / esp8266-fastled-webserver

GNU General Public License v3.0
713 stars 359 forks source link

Bug Fixes, More Patterns, and improovements #189

Open H3wastooshort opened 3 years ago

jasoncoon commented 3 years ago

Thank you for the PR! I need to add preprocessor directives for enabling/disabling websockets and IR, as some people have had stability and flicker problems with them enabled. I'll try to get this reviewed soon. Thanks!

henrygab commented 2 years ago

@HACKER-3000 ... Are you still interested in pursuing this change?

H3wastooshort commented 2 years ago

i tried cleaning it up a bit. i hope i didn't miss anything

henrygab commented 2 years ago

@HACKER-3000 ... Well, there were one or two things that needed doing. :smile:

There was a massive set of changes in the last couple months, and those had to get merged first.

I reverted the addition of the palettes... (expand)

Reviewing the code, the inclusion of the gradient palettes directly in the overall palettes structure had a fairly significant impact on RAM. This is because every gradient palette was expanded from ROM into a 64-byte version, since the palettes require exactly 16 CRGB values. The code worked, but there's likely a better way to do this, that can minimize the overhead. For example, perhaps the palettes that are exposed via the UI can be the combined set of normal 16-entry palettes + the gradient palettes. If the user selects a gradient palette, then it's converted on-the-fly to the 16-entry palette. This will avoid the second (expanded) copy of all the gradient palettes from being in memory.

I think the changes for the new patterns remain.

Can you verify the other bug fixes made it through the massive merge alright?

henrygab commented 2 years ago

@jasoncoon -- Can you review this, as I made a number of changes, so a second set of eyes would be appreciated?

jasoncoon commented 2 years ago

These changes look fine to me, but I won't be able to test them on an actual device until tomorrow evening at the earliest. And, even then, I don't plan on testing with IR or websockets enabled.

henrygab commented 2 years ago

These changes look fine to me, but I won't be able to test them on an actual device until tomorrow evening at the earliest. And, even then, I don't plan on testing with IR or websockets enabled.

Of course you wouldn't test IR or websockets ... in fact, they are explicitly marked as unsupported:

https://github.com/jasoncoon/esp8266-fastled-webserver/blob/8f7cfa549451cb8e078b977206349eff494345d1/esp8266-fastled-webserver/config.h#L140 https://github.com/jasoncoon/esp8266-fastled-webserver/blob/8f7cfa549451cb8e078b977206349eff494345d1/esp8266-fastled-webserver/config.h#L195

My changes were simply to move those bits of code into their own files as much as possible, to make maintenance easier, without impacting the binary.

H3wastooshort commented 2 years ago

im sorry if i caused trouble by re-opening discussion on this mess. i completely forgot how messy this was.

henrygab commented 2 years ago

im sorry if i caused trouble ...

There is no trouble. Not all changes go in, but all changes are considered! Thanks for opening the PR!