Notes about integrating New (current) versions of ESPAsyncServer and ESPAsyncWeb #690
Replies: 9 comments 23 replies
-
|
@robertlipe That's very interesting, thanks for raising this! As for the reason for the ESPAsyncWebServer fork, there is exactly one commit in the PSL tree, being this one: PlummersSoftwareLLC/ESPAsyncWebServer@a6cd518. (Spoiler alert: it removes one dependency called "Hash".) If upstream now builds and runs, then I'd say we should just roll with that from now on - once we have this working without turning off the fire alarm, of course. The template madness (no offense taken) is mine, and at least part of what the build is now tripping over seems to boil down to something within the call stack making one parameter to a function template const where it currently isn't - even though our code calls the function with a non-const parameter. I will have to look into that when I have the time. |
Beta Was this translation helpful? Give feedback.
-
|
Hey, Rutger!
I don't even know what reads library.json or what it is, though I do now
see them in .pio/libdeps/*/*/library.json so maybe that's something that
will neeed attention before formally adopting this.
I found a couple of discussions that described the needed changes, notably
ESP32Async/ESPAsyncWebServer#12
but our issues seem deeper than that. Your description sounds like you're
familiar with the issues and the solution here. i'm secure enough in my
programmer masculinity to admit that I poked at it for a few hours and got
nowhere. It's likely that stereotype 'one token gives you 100k of template
vomit' problem, but I couldn't spot it. Given your familiarity with the
code, I'd welcome a hand with this task. I agree that it can proceed
independently of the remaining Arduino3 transition.
The original library was very well known, but it also well known issues on
performance and stability that negatively impacted a LOT of ESP32 projects,
including several under constraints that you and I have bumped into quite
harshly. There have been several forks of it that have claimed to fix some
aspects of it, but only fairly recently has me-no-dev really said that he
just didn't have the time to keep working on it. MatthewCaribou was the
author of one such and a name I've seen on other project and his name is
familiar enough that I've seen enough of his code to respect his work. This
new fork seems to be a collection of Matthew's fork and that of a few
others, so it comes out of the gate strong. The result seems to be a
merging of a couple of such forks, though once successor that was on my
shopping list for replacement, PsychicHttpd
<https://github.com/hoeken/PsychicHttp>, as used in WLED, isn't one of
them People that adopted Matthew's work report measurable improvements in
RAM and performance
<https://forum.arduino.cc/t/ide-library-manager-handling-of-duplicate-library-names/1306491/9>
of "a few seconds", so I think there's more crunchy goodness
<https://github.com/ESP32Async/ESPAsyncWebServer/wiki#changes-in-this-repository>
here than a minor version number bump promises. There should be nice payoff
for us beyond Json7/Arduino3/G++ >8.
So, yes, if you can schedule this for some attention, I'd appreciate that.
We can merge it independently. I'm happy to work together on any issues
that remain. While the new code introduces CI testing to reduce breakage
from this point on, I know this kind of change is sensitive to how it's
called and the code it interoperates with, so I'm expecting there to be
more weirdness that I just didn't notice. It seemed to work well enough to
at least bring it to the table as a demo/request for help.
Getting started should be as easy as changing those two lines in pla*ini
and running a pio pkg update. The next build of a -e that has networking
enabled will fetch the packages and install them. With -fpermissive added,
it'll compile and run. (I can't remember if I added the const Asyncthingy*
changes at that point or not.) The entry ticket to this fun is pretty low.
I welcome the hand. This lets me move ahead with Arduino3 efforts in
environments that use networking as the existing stack splatters pretty
violently. This is a pretty nice outcome and you being familiar with the
issue that's blocking the replacement and willing to look at it is just a
cherry on top.
Thanx!
RJL
P.S. Displaying the template errors on a wide screen helped their
readability. Clang++ might be an option for is in the fugure, too...
…On Thu, Feb 13, 2025 at 7:38 AM Rutger van Bergen ***@***.***> wrote:
@robertlipe <https://github.com/robertlipe> That's very interesting,
thanks for raising this!
As for the reason for the ESPAsyncWebServer fork, there is exactly one
commit in the PSL tree, being this one:
***@***.***
<PlummersSoftwareLLC/ESPAsyncWebServer@a6cd518>.
(Spoiler alert: it removes one dependency called "Hash".)
If upstream now builds and runs, then I'd say we should just roll with
that from now on - once we have this working without turning off the fire
alarm, of course.
The template madness (no offense taken) is mine, and at least part of what
the build is now tripping over seems to boil down to something within the
call stack making one parameter to a function template const where it
currently isn't - even though our code calls the function with a non-const
parameter. I will have to look into that when I have the time.
—
Reply to this email directly, view it on GitHub
<#690 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD3YK7JBX5LLUZATG35T2PSN5NAVCNFSM6AAAAABXBOAZYSVHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTEMJYG44TMMA>
.
You are receiving this because you were mentioned.Message ID:
<PlummersSoftwareLLC/NightDriverStrip/repo-discussions/690/comments/12187960
@github.com>
|
Beta Was this translation helpful? Give feedback.
-
|
You're welcome! I actually think the number of compiler instantiations of the various templates isn't that extensive, but they interact in a way I don't currently quite fully understand. It's way too soon for me to throw in the towel on it though, as I only spent 10 minutes on it yesterday. This weekend should give me some more time to explore this; I won't waste further screen real-estate or your reading time on more text from my hand until I have something more substantial to report. |
Beta Was this translation helpful? Give feedback.
-
|
@robertlipe I had a sudden hunch this afternoon about what could be the problem I was running into, and I now have at least the Spectrum and Mesmerizer builds succeeding locally with the ESP32Async version of ESPAsyncWebServer. On top of that, GitHub CI claims that the builds of all envs actually work. The changes that make it work in my office are in the update-webserver branch in my fork, and included in this commit. I did not test the result in any way, shape or form, so it could well be that the build generates an image that does nothing or eats your dog. |
Beta Was this translation helpful? Give feedback.
-
|
I`m sitting in front of my mesmerizer, i uploaded the mesmerizer of your fork. It is working, but there is something strange. I have put al the credentials and api key and other stuf in secrets. What catches my eye is that the open weather maps api key is not displayed in the Device Settings Configuration. And everytime i put the api key in the Device Settings Configuration and apply. It does not work. If you go back in the Device Settings Configuration the key is gone. Also if i apply and reboot. So That little puppie dissapears. Harrie ;) |
Beta Was this translation helpful? Give feedback.
-
|
Thanx for the boost, Rutger. I wasn't even close in my approaches.
Templates are still an achilles heel for me.
Harrie, that's valuable testing. That was a case I had't exercised that
does have a unique path through the network fetcher and in this very PR, so
that's a path to debug. Welcome to the pre-cutting edge. I appreciate the
assist here, too.
Thanx, gents.
…On Sat, Feb 15, 2025 at 5:16 PM Harke Bosgraaf ***@***.***> wrote:
I`m sitting in front of my mesmerizer, i uploaded the mesmerizer of your
fork. It is working, but there is something strange. I have put al the
credentials and api key and other stuf in secrets. What catches my eye is
that the open weather maps api key is not displayed in the Device Settings
Configuration. And everytime i put the api key in the Device Settings
Configuration and apply. It does not work. If you go back in the Device
Settings Configuration the key is gone. Also if i apply and reboot. So That
little puppie dissapears.
Harrie ;)
—
Reply to this email directly, view it on GitHub
<#690 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD34HWHW3RQAGBUOKIEL2P7DDHAVCNFSM6AAAAABXBOAZYSVHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTEMRRGIZTQMY>
.
You are receiving this because you were mentioned.Message ID:
<PlummersSoftwareLLC/NightDriverStrip/repo-discussions/690/comments/12212383
@github.com>
|
Beta Was this translation helpful? Give feedback.
-
@robertlipe You're welcome. In the end, the whole problem was that the query parameter objects you get from request objects are now In any case, one could argue I over-engineered the solution by also making the request object that's passed as a parameter to the function templates itself @heidepiek As Robert already said, in the first place many thanks for picking up the gauntlet regarding testing! You may very well be on to something, but there is something specific about the weather API key: it will never be shown in the UI after saving (as in, it never has), by design. We consider the API key a secret, and like with web applications that allow you to change your password, the current password is never retrieved (in fact, with passwords they shouldn't even be saved, but that's a different story...) So, not seeing the weather API key in the UI is normal, but it should still be saved in the settings. That is to say that if you enter a valid weather API key, the weather effect should work. If it doesn't, then you have indeed found a bug introduced by the web server library update. |
Beta Was this translation helpful? Give feedback.
-
|
After a night's sleep, I just uploaded a fresh compilation of the Mesmerizer (Rutgers) to my board . I verified the API on my OpenWeatherMaps account that I use for my Mesmerizer and found that the weather effect isn't working. I wasn’t aware that the API key isn’t visible in the UI after saving🙈 , but I assume it should still be stored correctly. So it's most likely a bug🐝 . |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for confirming that. Do the other settings persist correctly? |
Beta Was this translation helpful? Give feedback.
-
|
I noticed that the sensitivity of the VU meter is slightly higher than in my other build. Also, when I adjust the brightness with the remote control, it doesn’t work the same way as in the other build. In the previous build, if I wanted to lower the brightness, I would press "off" on the remote control a few times, and the brightness would gradually decrease. When I pressed "on," it would go to full brightness. In your build, "off" immediately sets the brightness to its lowest, and "on" sets it to full brightness. That’s what stands out to me at the moment. Other than that, I haven’t noticed anything else yet. |
Beta Was this translation helpful? Give feedback.
-
|
Alright, I guess I'll get out my own Mesmerizer tomorrow and see what I conclude stil works, and what got broken. |
Beta Was this translation helpful? Give feedback.
-
|
Try pressing and holding OFF, if that doesn't dim, then there’s a new bug.
You can also install a jumper on the back of the PCB in one of 2 positions (or absent) to adjust mic sensitivity, but it shouldn’t need it.
- Dave
… On Feb 16, 2025, at 8:42 AM, Harke Bosgraaf ***@***.***> wrote:
I noticed that the sensitivity of the VU meter is slightly higher than in my other build. Also, when I adjust the brightness with the remote control, it doesn’t work the same way as in the other build.
In the previous build, if I wanted to lower the brightness, I would press "off" on the remote control a few times, and the brightness would gradually decrease. When I pressed "on," it would go to full brightness.
In your build, "off" immediately sets the brightness to its lowest, and "on" sets it to full brightness. That’s what stands out to me at the moment. Other than that, I haven’t noticed anything else yet.
—
Reply to this email directly, view it on GitHub <#690 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF5BMTSTODTBRVQNNFD2QCWXNAVCNFSM6AAAAABXBOAZYSVHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTEMRRGYYDIOA>.
You are receiving this because you are subscribed to this thread.
|
Beta Was this translation helpful? Give feedback.
-
|
Hello Dave, If I hold the button down, it immediately goes to the lowest brightness. Regarding the sensitivity of the VU, it’s possible that I adjusted it slightly in the previous build. But that was quite a while ago. My motto: If something works well, don’t touch it. I don’t have your PCB; I’m using a homemade version with the Lolin ESP32. But that shouldn’t make any difference in functionality—I haven’t changed the pinout, and the Lolin uses the same chip. The previous build works perfectly, but I thought I’d give Rutger’s fork a try. I figured they’d appreciate someone testing the changes. These are my findings—I’ve compared both builds countless times now, and these are really the issues I’m running into. 😌 Harrie :) |
Beta Was this translation helpful? Give feedback.
-
|
@heidepiek You're sure you're comparing the current tree in PlummersSoftwareLLC/NightDriversStrip:main to my update-webserver branch, right? I'm asking because if you look at the differences between the two, there are actually not that many changes made, in only 3 files, and none of them have anything to do with the handling of the IR remote - that logic follows completely different code paths that aren't touched. It also doesn't rely on the webserver library that is updated. Concerning the weather effect: it is important to know that when the API key is changed, it can take up to 10 minutes before the weather data is updated (or an attempt is made to do so, to be more precise.) A reboot about 30 seconds after updating the API key should also apply the change immediately after restart. |
Beta Was this translation helpful? Give feedback.
-
|
I Harrie |
Beta Was this translation helpful? Give feedback.
-
|
Have flashed my board with the main branche, i get this in the serial: Please build project in debug configuration to get more details about an exception. --- Terminal on COM11 | 115200 8-N-1 rst:0x1 (POWERON_RESET),boot:0x17 (SPI_FAST_FLASH_BOOT)
Starting SmartMatrix Mallocs
Stocks not working and also weather not working. Why?? I don`t know. I have yet to test your branche rutger. |
Beta Was this translation helpful? Give feedback.
-
|
The code is the best place to look. For matrix-based projects, it allows local dimming. For all others, it acts as an OFF button. This likely changed in the SPIRALLAMP days to provide an OFF feature.
…-Dave
else if (IR_OFF == result)
{
#if HUB75
deviceConfig.SetBrightness((int)deviceConfig.GetBrightness() - BRIGHTNESS_STEP);
#else
effectManager.ClearRemoteColor();
effectManager.SetInterval(0);
effectManager.StartEffect();
deviceConfig.SetBrightness(0);
#endif
return;
}
On Feb 16, 2025, at 9:08 AM, Harke Bosgraaf ***@***.***> wrote:
Hello Dave,
If I hold the button down, it immediately goes to the lowest brightness.
Regarding the sensitivity of the VU, it’s possible that I adjusted it slightly in the previous build. But that was quite a while ago. My motto: If something works well, don’t touch it.
I don’t have your PCB; I’m using a homemade version with the Lolin ESP32. But that shouldn’t make any difference in functionality—I haven’t changed the pinout, and the Lolin uses the same chip.
The previous build works perfectly, but I thought I’d give Rutger’s fork a try. I figured they’d appreciate someone testing the changes.
These are my findings—I’ve compared both builds countless times now, and these are really the issues I’m running into. 😌
Harrie :)
—
Reply to this email directly, view it on GitHub <#690 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF3JJJZVTVZZYDQTWRD2QCZW5AVCNFSM6AAAAABXBOAZYSVHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTEMRRGYYTSMQ>.
You are receiving this because you commented.
|
Beta Was this translation helpful? Give feedback.
-
|
Thanks!
In that case, it may well be that the bugs you found (at least those
related to the remote control) were introduced a while back. The weather
effect API key thing may still be caused by the web server library update.
…On Sun, 23 Feb 2025 at 13:57, Harke Bosgraaf ***@***.***> wrote:
Im going to test this imediatly! The current tree is probably not running
on my two mesmerizers. I saw this discussion, and uploaded your tree to a
third board i have laying around and tested it. And that is how i noticed
that the weather did not work. So bare with me, im going to test it!
Harrie
—
Reply to this email directly, view it on GitHub
<#690 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACPRHATXNAFVL5ZBBDEUP432RHAS5AVCNFSM6AAAAABXBOAZYSVHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTEMRZGE2DKMY>
.
You are receiving this because you commented.Message ID:
<PlummersSoftwareLLC/NightDriverStrip/repo-discussions/690/comments/12291453
@github.com>
|
Beta Was this translation helpful? Give feedback.
-
|
Rutger, sorry for wasting your valuable time! I feel like such a fool!😞 I can't believe I didn't see that. In so many of my branches that I have saved, the credentials are stored like that. I've had a similar issue in the past with a build from someone who made a stock ticker. Back then, I just copied and pasted the secrets.h file into the include folder. That also went wrong because some things had changed in secrets.h. So my build didn’t work either. I assumed the secrets folder was correct. Not to make excuses, but I also have other little projects here, and they use this: (https://openweathermap.org/city/2752520) How that ended up in secrets.h, beats me! Now that my older versions are working again, I tried your branch again. And now it works (I wonder why that is🙈 ). After a reboot, it takes about 15 seconds to think, but then the weather data appears. The remote and the sensitivity of the VU meter are still different from my current mesmerizer, though. Once again, sorry, Rutger! What a great test pilot I am! https://drive.google.com/file/d/1BeX0lrYmVpcvOX5HxpQufX-vCmtYcCt6/view?usp=sharing |
Beta Was this translation helpful? Give feedback.
-
|
I also looked into the brightness thing, and I think I found the bug. On November 1st last year, #660 got merged. That includes a change in remotecontrol.cpp that turns the brightness all the way down to 0 when the OFF button is pressed for all projects that don't use a HUB75 matrix. On Mesmerizers (i.e. the only project with a HUB75 matrix) the old behavior should remain: OFF gradually lowers the brightness. This is exactly what Dave said earlier, but now we know when this difference in behavior between Mesmerizer and other projects got introduced. The problem is that the new code should apply the same logic as before to Mesmerizer, but it doesn't, because the It's this code: NightDriverStrip/src/remotecontrol.cpp Lines 94 to 105 in 551dc02 Line 96 checks if I'll put a small PR together to fix this. |
Beta Was this translation helpful? Give feedback.
-
No, that's fine. I really, really appreciate you making the effort to test this stuff so thoroughly. And let's not forget, you did find a bug in the remote control handling (the PR I opened to fix that is #694, you could test that by building and flashing my fix-660-regression branch! :) ) and you established the stock data proxy server is down. Still two good catches, I would say!
I'm sorry to hear that. Sometimes things can indeed be that way... |
Beta Was this translation helpful? Give feedback.
-
|
Update on the stock data proxy server: that is indeed down and is very likely to be like that for another few days at most. Regular service will resume shortly, though. |
Beta Was this translation helpful? Give feedback.
-
|
I tried your fix-660-regression, bug solved👍 brightness.mp4Thanks rutger 😌 Fijne avond gewenst 👊 |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for fixing it!
- Dave
… On Feb 23, 2025, at 8:40 AM, Rutger van Bergen ***@***.***> wrote:
. On Mesmerizers (i.e. the only project with a HUB75 matrix) the old behavior should remain: OFF gradually lowers the brightness.
This is exactly what Dave said earlier, but now we know when this difference in behavior between Mesmerizer and other projects got introduced.
The problem is that the new code should apply the same logic as before to Mesmerizer, but it doesn't, because the #if statement on line 96 checks for the wrong define.
|
Beta Was this translation helpful? Give feedback.
-
|
@robertlipe Based on the tests that have now been performed, I think I'm going to push this to the main tree on the short term. I want to merge #694 first and bake a release before I do, just so we have a defined place we can return to if we missed something significant while working on this. Thanks again for raising this, I will add you as a co-author when I merge the PR. |
Beta Was this translation helpful? Give feedback.
-
|
Thank you everyone for your help. This change was landed into the tree successfully because we ganged up on it together. That's how open source works! |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Note to myself and/or peers. I'm going to put this note over here until one of
us has time to tame some template madness in include/webserver.h I'm going to
keep on with my other work and pick this up last, giving someone else time to
help (please 😀) or to at least announce my plan for landing this when it comes to
move our C++ compiler forward from 2018.
We've known our networ stack has been abandoned by their creator (me-no-dev)for
the last 2-34years. We know these modules were going to need maintenance before
we can move other parts of the code forward. It turns out that a a pair of maintainers
has stepped forard with an updated replacement version that has already fixed a
couple dozen long-standing data loss and other errors while being mostly compatible!
Some of the issues changed have been errors in low memeory handling, which is an
area of historic pain to us. (I haven't tested - and don't plan to - specific
cases. This is about general maintenance,)
Making the actual change is dead easy:
NOTE: We need to loop back and see why we were using a n EspAsyncWebServer from
Davee's imaster.
Here's the kink. It introduces a couple of warnings in our own networking code that Ive
started at for a ew hours and can't work through. The good news is that turning thes
warnings off (yes, I know this is taking down a smoke detector that's going off...)
actually allows the code to work really well.
With that in place, it all (well, everything I touched) passes quite well:
Tested:
Web interface
Telnet
OTA
nc 192.168.2.70 12000 | ledcat --geometry 25x25 show
(It's easier to read when put on a wiiiiide display.) Notes of the observed warnings:
Beta Was this translation helpful? Give feedback.
All reactions