PlummersSoftwareLLC / NightDriverStrip

NightDriver client for ESP32
https://plummerssoftwarellc.github.io/NightDriverStrip/
GNU General Public License v3.0
1.33k stars 215 forks source link

Build failures with FastLED 3.9.0 and 3.9.1 #661

Open rbergen opened 1 week ago

rbergen commented 1 week ago

--- Updated on 2024-11-03 to reflect the now current status ---

For a number of days starting 2024-10-27, we've seen build failures with the then current code in the main branch, that being a codebase that compiled successfully across projects/environments three weeks before.

After investigating the matter, these failures turned out to be related to 3.9.x releases of our dependency FastLED, the first of which (3.9.0) was indeed released around the mentioned date. We've witnessed different errors across FastLED versions 3.9.0 and 3.9.1, with the errors with FastLED 3.9.1 possibly being fixed by a change in FastLED 3.9.2, which was not yet released when this issue was originally opened.

Version 3.9.2 is available now, which may allow successful builds of NightDriverStrip again. As the 3.9.x range of versions is also marked as beta versions for 4.0, we decided to stay on version 3.8.0 for now. To achieve this, we pinned the FastLED dependency to that version in platformio.ini in #660.

People who do not wish to upgrade to the last version of main and run into "sudden" build failures that they cannot attribute to code changes they've made can try pinning down the FastLED version to 3.8.0 as well.

This can be done by changing the lib_deps line in platformio.ini that pulls in FastLED (line 52 in the former main branch) from this:

                  fastled/FastLED               @ ^3.4.0

to this:

                  fastled/FastLED               @ 3.8.0

Although this issue has now been resolved in the practical sense, we'll leave it open until we've "unpinned" the FastLED version once again.

robertlipe commented 1 week ago

Open a ticket with FastLED. I think 4.0 was released just a few days ago and Zach Vorhies (sometimes under the name zachees) is pretty interested in pushing FastLED forward in interesting ways once again. Cc him on this issue or make a new one on their github, etc.

Bonus points for nudging the open ticket about "lots" of pixels (lots of. Controller pins?) on S3.

Typed from a beach...

On Wed, Oct 30, 2024, 8:17 AM Rutger van Bergen @.***> wrote:

Since about 3 days we've been seeing build failures with the current code in the main branch, that being a codebase that compiled successfully across projects/environments three weeks ago.

After investigating the matter, these failures seem to be related to 3.9.x releases of our dependency FastLED, the first of which (3.9.0) was indeed released 3 days ago. We've witnessed different errors across FastLED versions 3.9.0 and 3.9.1, with the errors with FastLED 3.9.1 possibly being fixed by a change in FastLED 3.9.2, which is not yet released at the time of writing.

While we continue investigating this, people who run into "sudden" build failures that they cannot attribute to code changes they've made can try pinning down the FastLED version to 3.8.0, which is the most recent FastLED version with which we've seen successful builds for NightDriverStrip.

This can be done by changing the lib_deps line in platformio.ini that pulls in FastLED (line 52 in the current main branch https://github.com/PlummersSoftwareLLC/NightDriverStrip/blob/26fe5c1a3de829a1a741a12a4437b851dcb62ae4/platformio.ini#L52) from this:

              fastled/FastLED               @ ^3.4.0

to this:

              fastled/FastLED               @ 3.8.0

We will update/comment on this issue as we develop our understanding of the issues we're seeing, and decide how to find a stable way to resolve it.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/661, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD34HDWSMM62NPSR3EHTZ6DE4TAVCNFSM6AAAAABQ37DHR2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGYZDGOJRG43TCMA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

rbergen commented 1 week ago

I'm happy to tag @zackees on this issue (there, I fixed it), but based on the quick follow up on 3.9.0 yielding 3.9.1 within a day or two and 3.9.2 already seemingly being in the works as well, I don't think he'll learn anything new from this text.

It looks to me like in the drive to move towards a 4.x version of FastLED some breaking changes were inadvertently introduced into what is a minor version upgrade. That shouldn't happen (my stance would be that a stable 3.9.0 release should not aim to also be a 4.0 beta release if only by the rules of semantic versioning), but I am sure we're not the only ones noticing this and commenting on it.

So we've decided by now to just pin our FastLED dependency to 3.8.0 for the time being, and wait for things with the 3.9.x releases to settle down.

Concerning #580, I do believe @zackees gave the OP a concrete approach to test and provide feedback on, which it seems to me didn't happen yet, going by the most recent comments on that issue.

robertlipe commented 1 week ago

Indeed, without reading the error text here (or reproducing since it's missing), but from the subject lines alone: https://github.com/FastLED/FastLED/issues/1763 and https://github.com/FastLED/FastLED/issues/1759 look to be in our near proximity, perhaps making NightDriverLED an "easy" automated test for FastLED to use as a presubmit for a non-trivial, real-world app using FastLED as we mingle both ArduinoJSON and Arduino's premiere (abandoned) Web server.

Re: #580: That busywork is not needed when following our build instructions OR when using our trunk (courtesy our #657) but I've not followed the volley between FastLED versioning. I was able to repro it in the past. We've had enough people tell us this was broken that we know it's a problem, but I've not tracked the recent details, nor do I know if subsequent FastLED versions have made it better. While it's probably best to leave this issue for the new compilation issues alone. I just wanted to toss it into the mix while you had the attention of the right people. (Zach)

The new stuff in 4.0 looks really good for us, with it finally using DMA as intended instead of spin-waiting on each draw() call to finish, so I'm hopeful that this merge landing noise is worth it. Breaks stink, but I'm glad to see that project moving forward again.

RJL

On Wed, Oct 30, 2024 at 2:38 PM Rutger van Bergen @.***> wrote:

I'm happy to tag @zackees https://github.com/zackees on this issue (there, I fixed it), but based on the quick follow up on 3.9.0 yielding 3.9.1 within a day or two and 3.9.2 already seemingly being in the works as well, I don't think he'll learn anything new from this text.

It looks to me like in the drive to move towards a 4.x version of FastLED some breaking changes were inadvertently introduced into what is a minor version upgrade. That shouldn't happen (my stance would be that a stable 3.9.0 release should not aim to also be a 4.0 beta release if only by the rules of semantic versioning), but I am sure we're not the only ones noticing this and commenting on it.

So we've decided by now to just pin our FastLED dependency to 3.8.0 for the time being, and wait for things with the 3.9.x releases to settle down.

Concerning #580 https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/580, I do believe @zackees https://github.com/zackees gave the OP a concrete approach to test and provide feedback on, which it seems to me didn't happen yet, going by the most recent comments on that issue.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/661#issuecomment-2448058738, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD34OT7GHOUUBKKOU5T3Z6ERSXAVCNFSM6AAAAABQ37DHR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBYGA2TQNZTHA . You are receiving this because you commented.Message ID: @.***>

rbergen commented 1 week ago

[...] perhaps making NightDriverLED an "easy" automated test for FastLED to use as a presubmit for a non-trivial, real-world app using FastLED as we mingle both ArduinoJSON and Arduino's premiere (abandoned) Web server.

There's a thought. If Zach is open to/interested in that, I'd be happy to provide support towards that, to the extent that I reasonably can.

Re: #580: That busywork is not needed when following our build instructions OR when using our trunk

True. I'm just observing that that issue is stuck, and with some stretch of the imagination one could read Zach's comment as the helper asking for help with the helping. It might unblock the issue, is my pragmatic thinking. But as you said, let's continue the conversation about #580 there.

The new stuff in 4.0 looks really good for us, with it finally using DMA as intended instead of spin-waiting on each draw() call to finish, so I'm hopeful that this merge landing noise is worth it.

I'm sure we'll find out in due course. :)

Breaks stink, but I'm glad to see that project moving forward again.

Some breaks are necessary and sometimes even very helpful, which is fine... if you can plan for them. With the upgrade to a higher major version that's what you should expect. 3.9.0 should be a compatible upgrade from 3.4.0 (the former version in our platformio.ini), and it just isn't. Not just because of the namespace conflicts with ArduinoJson and ESPAsyncWebServer, but also because of newly introduced "ambiguous conversion from CRGB to uint32_t" errors (I'm probably paraphrasing the compiler's exact complaint). That one was pretty easy to address, but still.

zackees commented 1 week ago

3.9.2 will be release later today. It should fix this problem, which is caused by fastled using the name fs.h in a global header namespace include which ESP async server also does, unfortunately. Feel free to pin your dependency to master branch at c3e13bcec4275f0b964db65fc9a5be2b3041b6ba and you should be fine.

Btw, 3.9.2 contains the ability to overclock your WS2812 chipset with a define like this:

#define FASTLED_OVERCLOCK 1.2
#include "FastLED.h"

I was able to overclock a cheap WS2812 by 25%. The originating user who discovered this was able to overclock 50% for his quality WS2812.

zackees commented 1 week ago

I need to create a test which is an aggregate of common modules that's cross platform.

rbergen commented 1 week ago

3.9.2 will be release later today. It should fix this problem, which is caused by fastled using the name fs.h in a global header namespace include which ESP async server also does, unfortunately.

I appreciate the response and the update. I think that for us, the real benefits are in the 4.0 changes, so I think we'll wait for that release for now - and then allow some time to make the changes in our codebase that are necessary to work with that version.

I need to create a test which is an aggregate of common modules that's cross platform.

I read the original version of this comment earlier, which stated that NightDriverStrip is not the solution for this, in your eyes. I just wanted to say that that's fair enough.

zackees commented 1 week ago

3.9.2 is out now. We now have a test for this and hopefully this will take care of the issue moving forward.