New sound engine by davepl · Pull Request #740 · PlummersSoftwareLLC/NightDriverStrip

davepl · 2025-08-09T14:22:18Z

Description

Replaces the old SoundAnalyzer class with a new interface and implementation. Updates all callers to use the new interface.

I read the contribution guidelines in CONTRIBUTING.md.
I understand the BlinkenPerBit metric, and maximized it in this PR.
I selected main as the target branch.
All code herein is subjected to the license terms in COPYING.txt.

…trip into pdpwopr

Pick up main

…trip into pdpwopr

robertlipe · 2025-08-09T17:35:11Z

    {
      int iFan = random(0, NUM_FANS);
-      int passes = random(1, g_Analyzer._VURatio);
+      int passes = random(1, (int)g_Analyzer.VURatio());


Plase avoid shadows.

Sorry, not familiar with the term. What's wrong here?

shadow variables are like when you have a variable named passes inside a lower scope that already has a variable named passes. This is legal, but unnecessarily confusing. (References always get the token of closest scope.

for (int i : eyes)
for (int i : more_eyes)
for (int i : still_more_eyes
if (something) return i;
print i;

The last two lines will always refer to something in still_more_eyes[]. It can lead to maintenance hassles. You decide to rename the second varible and 150 lines down you are writing to it and forget to change that reference. Now you're writing to the outer one.

It's nefarious enough that gcc has a collection of -Wshadow warning options.

I get that, but I guess I just see it? Which variable is duplicated in an inner scope?

i guess shadows are aliasing as opposed to a brightness alpha spectra

@davepl It's passes. It's redefined in a for loop that compares its indexer variable with a passes that's defined in an outer context.

robertlipe · 2025-08-09T17:39:30Z

-{                                           //   reference them in g_Analyzer
-};
+#ifndef NUM_BANDS
+#define NUM_BANDS 16 // Default value for projects without audio


Consider describing why we need bands in a file named "sound" in a block "!ENABLE_AUDIO"

robertlipe · 2025-08-09T17:56:45Z

-    static constexpr size_t _sampling_period_us = PERIOD_FROM_FREQ(SAMPLING_FREQUENCY);
+    static inline size_t SamplingPeriodMicros()
+    {
+        return (size_t)PERIOD_FROM_FREQ(SAMPLING_FREQUENCY);


P_F_F() is only used twice, and both in integer context. Why not just make it in inline function that's strongly typed?

robertlipe · 2025-08-09T18:08:18Z

+        if (frameMax > _energyMaxEnv)
+            _energyMaxEnv = frameMax;
+        else
+            _energyMaxEnv = std::max(ENERGY_MIN_ENV, _energyMaxEnv * ENERGY_ENV_DECAY);


Is this block actually:
eME = std::max({E_M_E, frameMax, _eME * E_E_D});

Why do you ask?

So much for passive voice.

"Please replace with the clearer, and possibly more efficent as we have a 'max' opcode, suggestion."

I admit I haven't reasoned through all the code that determines frameMax to determine if what follows will ever happen, but the outcome of the current logic differs from std::max({_energyMaxEnv, frameMax, _energyMaxEnv * ENERGY_ENV_DECAY}) if frameMax is smaller than energyMaxEnv * ENERGY_ENV_DECAY.

Ah, now that I can get behind. Thank you. The key isn't using the second max to enforce a maximum, it's to enforce that the decayed rate never falls below MIN_ENV. I saw "max" and was reading that this was computing a maximum of a big number. It's just the maximum of a small number. Got it.

Withdrawn.

robertlipe · 2025-08-09T18:10:49Z

-                _vPeaks[i] = 0.0f;
+            float vNorm = _vPeaks[b] * invEnv;
+            if (vNorm > vMaxNorm)
+                vMaxNorm = vNorm;


Is this vMaxNorm = max_element(_vpeaks) * invEnv?

(Technically not as there are some differences in the overflow case, but I suspect if you're overflowing floats in this, you're living wrong.

robertlipe · 2025-08-09T18:27:29Z

-            ESP_ERROR_CHECK( i2s_start(I2S_NUM_0) );
-
-    #else
+#elif ELECROW || USE_I2S_AUDIO


While you're in the mode to test all the things AND have your head around config, if you can figure out what "ELECTROW" really really means here and can push I2S_foo_PIN into that config file and OUT of this code, that'd be great. Similarly, if yo can fold this case and the MESMERIZER/SPECTRUM . I whacked one of three nearly identical copies of this, but lacked the confidence/background to get it down to one. I don't think they're really different enough to to be two different blocks just to set the pin number via pin_config block

robertlipe · 2025-08-09T18:31:19Z

+            _vPeaks[i] = _Peaks._Level[i];
+        float sum = 0.0f;
+        for (int i = 0; i < NUM_BANDS; i++)
+            sum += _vPeaks[i];


Can this be done in one pass?
float sum = 0.0f;
for (int i = 0; i < NUM_BANDS; ++i) {
_vPeaks[i] = _Peaks._Level[i];
sum += _vPeaks[i];
}

robertlipe · 2025-08-09T18:32:51Z

+#elif USE_M5
+            _MicMode = PeakData::M5;
+#else
+            _MicMode = PeakData::MESMERIZERMIC;


Naming nit that's always bugged me: Is the else really "MESMERIZER" or is it "everything else"?

It's really "setups that run analog audio to a GPIO rather than an I2S source". But that's usually a mesmerizer, unless it's an umbrella :-)

In your garage, that may well be true. When someone else is trying to figure out what their prefab boad with a mic should map to or are soldering up a board of their own and trying to decide between an INMP441 or a MAX4466, it's probably not obvious to them that they need to send this code down the path named "mesmerizer" for an analog mic and "not m5 and not mesmerizer" for the 441.

We know these things were named for whatever you were building at the time, but it's an challenge to the DIY'ers in our discussion groups to get audio going.

robertlipe · 2025-08-09T18:34:26Z

-                sum += _Peaks[i];
-
-            // Scale it so that it is not always in the top red
+                sum += _Peaks._Level[i];


There are lots of these loops that can be:
float sum = std::accumulate(_Peaks._Level, _Peaks._Level + NUM_BANDS, 0.0f);

Are you certain /Ofast isn't already that clever? Would be good to know. I don't mind optimizing it, but it's more readable expressed as two passes.

It can't be that clever under the "as if" rule. It would have to prove that sum never overflows. Poking it, range analysis also seems to factor in, though I think it can prove that sum isn't aliased and that the arrays don't overlap, but you can see foryourself:

https://godbolt.org/z/x8e9xnPf1

Unfortunately, teh zero-overhead-loop end label is eaten by Goldbolt. There's an open BR on that at compiler-explorer/compiler-explorer#6496. Because this code is so trivial, if you know the return value (tally) is in a2, just imagine every add with a destination of a2 to be followed by the closes preceeding LOOP insn and it'll all make sense.

Even without that, it's hopefully clear that in a code path where fps matters, one loop over NUM_BANDS is better than two loops over NUM_BANDS.

robertlipe · 2025-08-11T03:22:31Z

There are two distinctly different "passes", as I said. On inside the loop, one just above it.

…

On Sun, Aug 10, 2025, 10:17 PM David W Plummer ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/effects/strip/faneffects.h <#740 (comment)> : > for (int iPass = 0; iPass < passes; iPass++) { int iFan = random(0, NUM_FANS); - int passes = random(1, g_Analyzer._VURatio); + int passes = random(1, (int)g_Analyzer.VURatio()); I get that, but I guess I just see it? Which variable is duplicated in an inner scope? — Reply to this email directly, view it on GitHub <#740 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD34V6V7WCQ36LHTD22T3NADLLAVCNFSM6AAAAACDP66VSCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTCMBTHEYDONZWGE> . You are receiving this because you commented.Message ID: ***@***.*** com>

davepl · 2025-08-11T14:18:38Z

I see iPass, iFan, and passes. One of each. Which is the shadow of what?

…

-Dave

On Aug 10, 2025, at 9:25 PM, Ghosteyezzzz ***@***.***> wrote: @Thermotronica commented on this pull request. In include/effects/strip/faneffects.h <#740 (comment)>: > for (int iPass = 0; iPass < passes; iPass++) { int iFan = random(0, NUM_FANS); - int passes = random(1, g_Analyzer._VURatio); + int passes = random(1, (int)g_Analyzer.VURatio()); i guess shadows are aliasing as opposed to a brightness alpha spectra — Reply to this email directly, view it on GitHub <#740 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF4N26Z7BQJPGNKA2WD3NALNHAVCNFSM6AAAAACDP66VSCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTCMBUGAYDSNJZHA>. You are receiving this because you authored the thread.

davepl · 2025-08-11T15:06:28Z

“In code not visible here”. That’s the key point. I see it if I go look in the actual file, but thought we were talking about the exceprt, do didn’t see it. Will fix!

…

On Aug 11, 2025, at 8:03 AM, Rutger van Bergen ***@***.***> wrote: @rbergen commented on this pull request. In include/effects/strip/faneffects.h <#740 (comment)>: > for (int iPass = 0; iPass < passes; iPass++) { int iFan = random(0, NUM_FANS); - int passes = random(1, g_Analyzer._VURatio); + int passes = random(1, (int)g_Analyzer.VURatio()); @davepl <https://github.com/davepl> It's passes. It's redefined in a for loop that compares its indexer variable with a passes that's defined in an outer context. — Reply to this email directly, view it on GitHub <#740 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF7Y35HHIPU6BTR7FUD3NCWFTAVCNFSM6AAAAACDP66VSCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTCMBWGMZDMOBTHE>. You are receiving this because you were mentioned.

rbergen · 2025-08-11T15:13:13Z

These e-mail responses at the end of the comment chain always get me...

I agree the limited scope of the code that is commented on doesn't help. (But in a very pedantic stance I do have to note that the passes in the condition clause of the for statement that is included in the excerpt refers to the outer passes - which contributes to the potential confusion in this particular case.)

Pull pdpwopr changes into my audio branch

This reverts commit e0e93f8.

davepl and others added 30 commits August 5, 2025 11:35

Add PDPWOPR project. PDPCMX effect

727636d

Clean up project, fix S3 screen crash

fad5ad6

Fix non-working python script dependency

f6f2068

Interim work on platformio.ini refactor

ca412f6

Fix audio break

06edf0e

Fix audio issue with elecrow mesmerizer

e23d3c6

Add libdeps, effects to mesmerizer

fb1fb79

Merge branch 'pdpwopr' of github.com:PlummersSoftwareLLC/NightDriverS…

8e0a5b4

…trip into pdpwopr

Fix partition table for mesmerizer

cc243da

Build fixes

264f500

Make them all build

9e3fa75

Before lib_deps

05ae704

Remove redundant libs

6146dc9

Move MESMERIZER out of globals as a test

a159aa3

Migrate globals to platformio

44852e8

Fix feather_hexagon

4d095d7

Merge pull request #739 from PlummersSoftwareLLC/main

b88a4d9

Pick up main

Fix audio on mesmerizer

1eb0425

Merge branch 'pdpwopr' of github.com:PlummersSoftwareLLC/NightDriverS…

c49b60e

…trip into pdpwopr

Update soundanalyzer define

82728e9

Work around IDF version differences

ff4720c

Fix missing endif

36aaa17

Fix (i2s_mode_t) issue

76bf1ad

Remove project default

7da880e

Enable Audio in spectrum effects

9ec664a

Enable audio in spectrum2

dcdd6f3

New sound engine

3ed6077

Revert to custom_effects.h overriding the effect flag

32d5625

24 band plus cleanup

4cb1584

Change data type to float

690b3be

robertlipe reviewed Aug 9, 2025

View reviewed changes

Dave Plummer added 3 commits August 9, 2025 12:54

Add comments

36f56c2

Per-mic audio settings

c062423

Redo smoothing

6ecdfd4

Merge branch 'audio' into pdpwopr

fa44d8a

davepl and others added 9 commits August 11, 2025 08:13

Merge pull request #741 from PlummersSoftwareLLC/pdpwopr

53925b6

Pull pdpwopr changes into my audio branch

Fix shadow of variable

e3746c9

FrameGate

e0e93f8

Revert "FrameGate"

c347cc9

This reverts commit e0e93f8.

Switch to cbrt() only

d4183a6

Fix shadow of variables

6c565c5

Update comment

fba2150

Disable warning, set to 24ksamples

4419a6c

Merge branch 'pdpwopr' into audio

25d55ff

davepl closed this Aug 12, 2025

robertlipe mentioned this pull request Aug 19, 2025

Templates - Migrate macros to templates, simplify effects table, add effectId() to all derived effects #748

Merged

4 tasks

rbergen deleted the audio branch December 26, 2025 21:24

Uh oh!

Conversation

davepl commented Aug 9, 2025

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertlipe commented Aug 11, 2025 via email

Uh oh!

davepl commented Aug 11, 2025 via email

Uh oh!

davepl commented Aug 11, 2025 via email

Uh oh!

rbergen commented Aug 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants