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

Audio Data UDP Sync (receive only) on ESP8266 #3960

Closed
gaaat98 opened this issue May 6, 2024 · 14 comments · Fixed by #3962
Closed

Audio Data UDP Sync (receive only) on ESP8266 #3960

gaaat98 opened this issue May 6, 2024 · 14 comments · Fixed by #3962

Comments

@gaaat98
Copy link
Contributor

gaaat98 commented May 6, 2024

I needed to sync the effects from an ESP32 with an I2S mic to an ESP8266 (actually, an Athom controller running on an ESP8285, but I believe it's almost the same). I was wondering why this feature is not implemented in mainline WLED, especially since the code is basically already there and does not require any particular dependency (I think).

Then I found MoonModules/pull/60 and successfully ported it to 0_15... It does the job and seems to be stable, so I am asking if there is any particular reason not to add this feature also here.

Thank you for your time, and pardon my inexperience!

@blazoncek
Copy link
Collaborator

It is not implemented since it was developed by MM contributors and collaborators and nobody wanted (or took the time) to port it back upstream.
The base issue IMO is the change of licensing model used in MM. They use GPL v3 while this repository uses MIT license. So you will need to get explicit permission to submit the ported code for MIT license.

@softhack007
Copy link
Collaborator

softhack007 commented May 7, 2024

Hi, basically @blazoncek is right.
The full story is, 8266 sound sync (PR 60) was a night hack in MM some months ago, to see if we can get it to basically work. We still had issues with watchdog reset during OTA, but I think that's fixed now. Another unclear point was random UDP connection loss, and we did not investigate this any further.

@gaaat98 thanks for the feedback, good to hear that its already working for you.

Supporting 8266 is a low prio in the MM fork, as the chip is just too slow for our interesting use cases, and 8266 will be EOL in 1-2 years anyway. That's why we put that topic aside for the moment.

So the main reasons for not (yet) porting it to upstream were

  • Time
  • Quality (very experimental code)
  • MIT license in upstream - we have changed our MM specific code to GPLv3 after some bad experiences with MIT licensing - copycats simply re-branding our code, but never giving anything back. That's really frustrating, I can tell you ...

In fact I'm just discussing with @netmindz (the other main author of PR 60) and maybe we can bring the code to upstream in a new PR from the MM fork. Let's see.

@gaaat98
Copy link
Contributor Author

gaaat98 commented May 7, 2024

Totally understandable points; thank you both for the clarification!

In fact I'm just discussing with @netmindz (the other main author of PR 60) and maybe we can bring the code to upstream in a new PR from the MM fork. Let's see.

Nice! IMO that's a very good feature that could be beneficial also to other users given how popular the 8266 is, especially for beginners.

@netmindz
Copy link
Collaborator

netmindz commented May 7, 2024

@gaaat98 The main reason I didn't go back and create a PR for upstream is that as I'm sure you found already that there is too much divergence to just cherry pick the changes and as Frank mentioned, in general MM does not spend any time on ESP8266.

I can't remember off the top of my head if that PR was before or after change of licence, but either way I hereby grant you permission to use my code in that PR for your own implementation in AirCookie

@netmindz
Copy link
Collaborator

netmindz commented May 7, 2024

Looking again at that PR I'm not sure why it's showing so many changes that don't directly relate to the work, it should be pretty much only changes inside the two files in the usermod directory for AudioReactive

It might be better to look commit by commit rather than the full pr

@netmindz
Copy link
Collaborator

netmindz commented May 7, 2024

Looks like some of the changes simply relate to fixing 8266 issues that related to the fact that we weren't doing any 8266 builds after we ended support

@gaaat98
Copy link
Contributor Author

gaaat98 commented May 7, 2024

@netmindz I can confirm that porting the feature is just a matter of porting your version of audio_reactive.h and audio_source.h and add the compile option for the audioreactive usermod in one of the esp8266 builds.

However, despite what I said earlier it is not as stable as I thought, it works fine on my sk6812 strip driven by esp8285 but crashes pretty frequently on my analog strip driven by another esp8285, I guess I have to investigate a bit before opening a PR

@netmindz
Copy link
Collaborator

netmindz commented May 7, 2024

@netmindz I can confirm that porting the feature is just a matter of porting your version of audio_reactive.h and audio_source.h and add the compile option for the audioreactive usermod in one of the esp8266 builds.

However, despite what I said earlier it is not as stable as I thought, it works fine on my sk6812 strip driven by esp8285 but crashes pretty frequently on my analog strip driven by another esp8285, I guess I have to investigate a bit before opening a PR

To be clear, I said only the changes in my pr you could take. You do not have permission to totally replace those two files in AirCookie with the MM version in totality

@gaaat98
Copy link
Contributor Author

gaaat98 commented May 7, 2024

To be clear, I said only the changes in my pr you could take. You do not have permission to totally replace those two files in AirCookie with the MM version in totality

Yes, of course! To quickly test I simply copied those files, wrongly assuming that the only changes were yours, but I am now realizing that it is not the case, I will need to put some work into it!

@softhack007
Copy link
Collaborator

@gaaat98 the relevant changes for 8266 are basicially

  • putting #ifdef ARDUINO_ARCH_ESP32 around audio code that's not needed for sync receive
  • slightly modified UDP calls
  • a replacement for onUpdateBegin()
  • settings page reduction
  • a few bugfixes after merging PR 60 (like MoonModules@523893b )

@softhack007
Copy link
Collaborator

softhack007 commented May 7, 2024

but I am now realizing that it is not the case, I will need to put some work into it!

That's the right approach 👍
MM audioreactive has evolved a lot compared to the Aircoookie version, and - in addition to the licensing incompatibility - it will not run as-is inside AC WLED.

You can expect all kinds of weird behaviour from your "drop-in replacement" - like stack smashing, writing unallocated memory, and a half-working usermod settings page. This is due to the MM version making assumptions that are not true any more in AC WLED.

So better to understand the changes we made for 8266 in MoonModules, and then bring only these changes into AC WLED. You can focus on audio_reactive.h - audio_source.h is irrelevant for 8266.

@gaaat98
Copy link
Contributor Author

gaaat98 commented May 8, 2024

I managed to port all the changes related to the PR, however I still had a lot of wdt resets on my analog strip... After a bit of investigation it seems like analog strips can only be used reliably when the clock is set to "Slowest", serial still shows a lot of loop delays ranging from 3ms to 13ms but at least the esp does not reboot . I also tried flashing the 160MHz version but I still couldn't increase the clock speed of the strip. I guess this is just a performance limitation of the esp8266

@blazoncek
Copy link
Collaborator

@gaaat98 open a draft PR so others can also have a look and contribute if needed.

@gaaat98
Copy link
Contributor Author

gaaat98 commented Jul 13, 2024

PR merged, issue closed

@gaaat98 gaaat98 closed this as completed Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants