Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug Fixes, More Patterns, and improovements #189

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

H3wastooshort
Copy link

No description provided.

@jasoncoon
Copy link
Owner

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!

Copy link
Collaborator

@henrygab henrygab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, let me be clear that I don't have rights in this repository.

If I owned this depot, I would request significant changes.

At a minimum:

  • Don't change default maximum amps in the depot -- for safety
  • Wrap IR and ArduinoUpdate capabilities to be conditionally enabled by defining compilation symbols
  • Don't enable functionality that's not quite ready (e.g., web sockets seems to only partially be there)

GradientPalettes.h Outdated Show resolved Hide resolved
GradientPalettes.h Outdated Show resolved Hide resolved
GradientPalettes.h Outdated Show resolved Hide resolved
GradientPalettes.h Outdated Show resolved Hide resolved
esp8266-fastled-webserver.ino Outdated Show resolved Hide resolved
esp8266-fastled-webserver.ino Outdated Show resolved Hide resolved
esp8266-fastled-webserver.ino Outdated Show resolved Hide resolved
esp8266-fastled-webserver.ino Outdated Show resolved Hide resolved
esp8266-fastled-webserver.ino Outdated Show resolved Hide resolved
esp8266-fastled-webserver.ino Outdated Show resolved Hide resolved
@henrygab
Copy link
Collaborator

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

@H3wastooshort
Copy link
Author

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

@henrygab
Copy link
Collaborator

henrygab commented Dec 13, 2021

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

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?

Commands.h Outdated Show resolved Hide resolved
@henrygab henrygab self-requested a review December 13, 2021 23:56
henrygab
henrygab previously approved these changes Dec 14, 2021
Copy link
Collaborator

@henrygab henrygab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, pending HACKER-3000 confirmation.

It does the following:

  • Adds a new gradient palette, named Vapour_gp
  • Adds six new effects:
    • currentPaletteTwinkles(), aka Palette Twinkles
    • coolerpalettebow(), aka PaletterBow
    • palettebowglitter(), aka PaletteBow With Glitter
    • palettebow(), aka Solid PaletteBow
    • firepal(), aka PaletteFire
    • waterpal(), aka PaletteWater
  • Moves websocket functionality to separate file, Websockets.cpp
  • Enables successful platformio builds when ENABLE_IR is defined as 1
  • Adds additional configuration and validation for ENABLE_IR, ENABLE_ARDUINO_OTA, and ENABLE_WEBSOCKETS
  • Add additional example of custom configuration to platformio_override_example.ini

Edit -- Note that ENABLE_IR, ENABLE_ARDUINO_OTA, and ENABLE_WEBSOCKETS are not currently supported or built by any default configuration.

@henrygab
Copy link
Collaborator

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

@jasoncoon
Copy link
Owner

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
Copy link
Collaborator

henrygab commented Dec 20, 2021

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:

#warning "ENABLE_IR is still in development, and is NOT supported"

#warning "ENABLE_WEBSOCKETS is still in development, and is NOT supported"

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
Copy link
Author

H3wastooshort commented Dec 20, 2021

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

@henrygab
Copy link
Collaborator

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants