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

Audiosync espnow #148

Draft
wants to merge 14 commits into
base: mdev
Choose a base branch
from
Draft

Conversation

netmindz
Copy link
Collaborator

No description provided.

@softhack007 softhack007 self-requested a review July 18, 2024 14:14
@softhack007 softhack007 self-assigned this Jul 18, 2024
@@ -960,17 +967,6 @@ class AudioReactive : public Usermod {
int8_t mclkPin = MCLK_PIN;
#endif
#endif
// new "V2" audiosync struct - 40 Bytes
Copy link
Collaborator

Choose a reason for hiding this comment

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

@netmindz something is strange with your PR - this a very old definition of audioSyncPacket, also I'm missing some other new features that were added in the last 4 months.

might be better if you make a fresh branch from mdev and drop your changed file into the latest codebase 🤔

@@ -1834,7 +1816,19 @@ class AudioReactive : public Usermod {
DEBUGSR_PRINTLN(F("AR connected(): old UDP connection closed."));
}

if (audioSyncPort > 0 && (audioSyncEnabled & 0x03)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this line looks different now

if ((audioSyncPort > 0) && (audioSyncEnabled > AUDIOSYNC_NONE)) {
#ifdef ARDUINO_ARCH_ESP32
udpSyncConnected = fftUdp.beginMulticast(IPAddress(239, 0, 0, 1), audioSyncPort);

@@ -442,7 +446,7 @@ void FFTcode(void * parameter)
// taskYIELD(), yield(), vTaskDelay() and esp_task_wdt_feed() didn't seem to work.

// Don't run FFT computing code if we're in Receive mode or in realtime mode
if (disableSoundProcessing || (audioSyncEnabled & 0x02)) {
if (disableSoundProcessing || (audioSyncEnabled == 2) || (audioSyncEnabled == 4)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of using "4", i'd suggest to add something like this

#define AUDIOSYNC_SEND_ESPNOW 0x09      // UDP sound sync - 9 = send by ESP_NOW
#define AUDIOSYNC_REC_ESPNOW 0x0A        // UDP sound sync - 10= receive by ESP_NOW
#define AUDIOSYNC_REC_ESPNOW 0x0D        // UDP sound sync - 14= receive-or-local by ESP_NOW

Or even simpler, let's add another variable bool audioSyncUseESPNOW; to indicate that ESP-NOW is used instead of UDP multicast.

Edit: I thing the second option is better.
And I won't cry about another byte of RAM used while there are still spare bits around somewhere 😉

@@ -928,6 +928,7 @@ build_flags_S =
lib_deps_S =
;; https://github.com/kosme/arduinoFFT#develop @ 1.9.2+sha.419d7b0 ;; used for USERMOD_AUDIOREACTIVE - using "known working" hash
https://github.com/softhack007/arduinoFFT.git#develop @ 1.9.2 ;; used for USERMOD_AUDIOREACTIVE - optimized version, 10% faster on -S2/-C3
https://github.com/RobTillaart/[email protected]
Copy link
Collaborator

@softhack007 softhack007 Jul 18, 2024

Choose a reason for hiding this comment

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

better add this to AR_lib_deps

AR_lib_deps = https://github.com/softhack007/arduinoFFT.git#develop @ 1.9.2 ;; used for USERMOD_AUDIOREACTIVE - optimized version, 10% faster on -S2/-C3

just make a second line so it looks like this

AR_lib_deps = 
    https://github.com/softhack007/arduinoFFT.git#develop @ 1.9.2      ;; used for USERMOD_AUDIOREACTIVE - optimized version, 10% faster on -S2/-C3
    https://github.com/RobTillaart/[email protected] ;; provides a running average filter

Copy link
Collaborator

@softhack007 softhack007 Jul 18, 2024

Choose a reason for hiding this comment

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

edit: need to think about this filter - it looks rather complicated maybe i can give you an easier "home-brew" one that serves the same purpose.

@netmindz
Copy link
Collaborator Author

Sorry @softhack007 - I didn't mean for you to look at this just yet. The branch is very old so I need to update and fix the conflicts etc before it's ready to be looked at

@softhack007
Copy link
Collaborator

Sorry @softhack007 - I didn't mean for you to look at this just yet. The branch is very old so I need to update and fix the conflicts etc before it's ready to be looked at

@netmindz no problem :-) actually its good to see an early preview 👍 , so I can make upfront recommendations instead of changing everything late in the process.

@netmindz
Copy link
Collaborator Author

I'd accidentally made the PR inside my own fork rather than into MoonModules last year

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

Successfully merging this pull request may close these issues.

2 participants