search

PlummersSoftwareLLC / NightDriverStrip

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

Make Audio Great Again #646

Closed davepl closed 3 months ago

davepl commented 3 months ago

Description

This fixed the issue with the M5 audio on various projects, which now works nicely. It also unifies a bunch of variables like noise floor and cutoff. Tested on M5 Stick C Plus, Plus 2, Stack Core 2, and Mesmerizer.

robertlipe commented 3 months ago

  • ifndef AUDIO_MIC_SCALAR

@robertlipe https://github.com/robertlipe Thanks for triggering my (own) language OCD. :)

It's actually not your OWN language, which makes it even better!

I think the argument should then be that it should be AUDIO_MIC_SCALE?

I could get behind that. Magnifier, Multiplier, amplitude, bias, boost, etc. are all in that basic basket but none of those seem quite right. It does seem like there's a better word for that, but at this hour, my own (native) language skills are failing me.

I believe the thing is that the M5 path works on the M5 devices and the I2S

path doesn't? But it could be that's an outdated perception of the state of affairs. I think we'd need @davepl https://github.com/davepl to clear that up.

I don't own any of these, so I'm not attuned to their zen. My angle is that I2S is a feature of the SOC, not really of the board it's on. I could understand M5 making a library that made the I2S interface easier or more Arduino-like or something, but I2S audio is a chip thing and not a board thing. If you have to have the native path ANYWAY (and I suggest we do, even ignoring Mesmerizer's high pole position for this discussion) my question is if we need the M5 path at all. If the difference is, say, that Mesmerizer uses a native I2S digital mic and the M5's are using analog mics into the A/D converter (which are known to be awful on the pre-2020 chips) then we're probably drawing the line in the wrong place and calling it the wrong thing.

I'd like to understand why the M5's are special. They're still plain ole Espressif chips and it's not like the I2S registers aren't there or are different or something.

The less board special casing we have and the fewer exernal libraries we rely on, the happier I think we'll all be. That's one thing we pretty regularly agree on.

rbergen commented 3 months ago

It's actually not your OWN language, which makes it even better!

Ha! :)

I could get behind that. Magnifier, Multiplier, amplitude, bias, boost, etc. are all in that basic basket but none of those seem quite right. It does seem like there's a better word for that, but at this hour, my own (native) language skills are failing me.

Acknowledging my own part in this up-front, my observation is that we've spent a lot of letters on a semantic matter (by the dictionary definition of the term), without finding a clearly satisfying alternative. Maybe we can leave it as it is until that alternative is found in a future moment of genius, and make it a hidden gem in a future PR. ;)

I don't own any of these, so I'm not attuned to their zen. My angle is that I2S is a feature of the SOC, not really of the board it's on.

I own some, but not the M5StickC Plus2. My understanding from the related Discussion and PR comments is that everything was honky-dory until that device showed up and everything ran off the rails with the former I2S-based implementation.

But again, I do use the word "understanding", and I could be mistaken.

I'd like to understand why the M5's are special.

That's a fair question, and I'm not sure we now know. I for sure don't.

The less board special casing we have and the fewer exernal libraries we rely on, the happier I think we'll all be. That's one thing we pretty regularly agree on.

We still do... as long as it works.

robertlipe commented 3 months ago

Let's at least change the spelling to the right word while this line is hot in our cerebral caches.

Id rather not buy one for study, but if that's what it takes, I can do so. It sure looks like that a plain ole ESP32-PICO-V3 module, so the i2s hardware should be the same as all the esp32-nothings.

If we have an issue with spm1423, we should have it on any board paired with that mic, not special cases for m5stick of the day. That's what https://docs.m5stack.com/en/core/M5StickC%20PLUS2 says that board is using, but it's also a little eval board that easy to attach to other boards.

If possible, I'd like to understand this at the electronics level and not marketing/convenience library level.

On Sat, Jul 27, 2024, 4:53 AM Rutger van Bergen @.***> wrote:

It's actually not your OWN language, which makes it even better!

Ha! :)

I could get behind that. Magnifier, Multiplier, amplitude, bias, boost, etc. are all in that basic basket but none of those seem quite right. It does seem like there's a better word for that, but at this hour, my own (native) language skills are failing me.

Acknowledging my own part in this up-front, my observation is that we've spent a lot of letters on a semantic matter (by the dictionary definition of the term), without finding a clearly satisfying alternative. Maybe we can leave it as it is until that alternative is found in a future moment of genius, and make it a hidden gem in a future PR. ;)

I don't own any of these, so I'm not attuned to their zen. My angle is that I2S is a feature of the SOC, not really of the board it's on.

I own some, but not the M5StickC Plus2. My understanding from the related Discussion and PR comments is that everything was honky-dory until that device showed up and everything ran of the rails with the former I2S-based implementation.

But again, I do use the word "understanding", and I could be mistaken.

I'd like to understand why the M5's are special.

That's a fair question, and I'm not sure we now know. I for sure don't.

The less board special casing we have and the fewer exernal libraries we rely on, the happier I think we'll all be. That's one thing we pretty regularly agree on.

We still do... as long as it works.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/646#issuecomment-2254097401, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD37TOEPCXNKHJEEDQ4DZONU27AVCNFSM6AAAAABLRHJYIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGA4TONBQGE . You are receiving this because you were mentioned.Message ID: @.***>

davepl commented 3 months ago

Not sure I understand the question - it’s fixed. It works. No special cases, all M5 hardware uses the same code.

What are we trying to solve?

On Jul 27, 2024, at 12:36 PM, Robert Lipe @.***> wrote:

Let's at least change the spelling to the right word while this line is hot in our cerebral caches.

Id rather not buy one for study, but if that's what it takes, I can do so. It sure looks like that a plain ole ESP32-PICO-V3 module, so the i2s hardware should be the same as all the esp32-nothings.

If we have an issue with spm1423, we should have it on any board paired with that mic, not special cases for m5stick of the day. That's what https://docs.m5stack.com/en/core/M5StickC%20PLUS2 says that board is using, but it's also a little eval board that easy to attach to other boards.

If possible, I'd like to understand this at the electronics level and not marketing/convenience library level.

On Sat, Jul 27, 2024, 4:53 AM Rutger van Bergen @.***> wrote:

It's actually not your OWN language, which makes it even better!

Ha! :)

I could get behind that. Magnifier, Multiplier, amplitude, bias, boost, etc. are all in that basic basket but none of those seem quite right. It does seem like there's a better word for that, but at this hour, my own (native) language skills are failing me.

Acknowledging my own part in this up-front, my observation is that we've spent a lot of letters on a semantic matter (by the dictionary definition of the term), without finding a clearly satisfying alternative. Maybe we can leave it as it is until that alternative is found in a future moment of genius, and make it a hidden gem in a future PR. ;)

I don't own any of these, so I'm not attuned to their zen. My angle is that I2S is a feature of the SOC, not really of the board it's on.

I own some, but not the M5StickC Plus2. My understanding from the related Discussion and PR comments is that everything was honky-dory until that device showed up and everything ran of the rails with the former I2S-based implementation.

But again, I do use the word "understanding", and I could be mistaken.

I'd like to understand why the M5's are special.

That's a fair question, and I'm not sure we now know. I for sure don't.

The less board special casing we have and the fewer exernal libraries we rely on, the happier I think we'll all be. That's one thing we pretty regularly agree on.

We still do... as long as it works.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/646#issuecomment-2254097401, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD37TOEPCXNKHJEEDQ4DZONU27AVCNFSM6AAAAABLRHJYIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGA4TONBQGE . You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/646#issuecomment-2254235850, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4HCFY764DRV6DJ4RJLD5TZOPZDJAVCNFSM6AAAAABLRHJYIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGIZTKOBVGA. You are receiving this because you were mentioned.

robertlipe commented 3 months ago

The question, again, Is why there's a special case at all. There's a lovely i2s abstraction already in esp-idf/arduino that's used by everything else. The m5s we care about use esp32, so why not just talk to the esp32 instead of bouncing through a vendor library.

Other devices might use that same i2s mic if it's actually the mic that's being specially cased.

On Sat, Jul 27, 2024, 3:26 PM David W Plummer @.***> wrote:

Not sure I understand the question - it’s fixed. It works. No special cases, all M5 hardware uses the same code.

What are we trying to solve?

  • Dave

On Jul 27, 2024, at 12:36 PM, Robert Lipe @.***> wrote:

Let's at least change the spelling to the right word while this line is hot in our cerebral caches.

Id rather not buy one for study, but if that's what it takes, I can do so. It sure looks like that a plain ole ESP32-PICO-V3 module, so the i2s hardware should be the same as all the esp32-nothings.

If we have an issue with spm1423, we should have it on any board paired with that mic, not special cases for m5stick of the day. That's what https://docs.m5stack.com/en/core/M5StickC%20PLUS2 says that board is using, but it's also a little eval board that easy to attach to other boards.

If possible, I'd like to understand this at the electronics level and not marketing/convenience library level.

On Sat, Jul 27, 2024, 4:53 AM Rutger van Bergen @.***> wrote:

It's actually not your OWN language, which makes it even better!

Ha! :)

I could get behind that. Magnifier, Multiplier, amplitude, bias, boost, etc. are all in that basic basket but none of those seem quite right. It does seem like there's a better word for that, but at this hour, my own (native) language skills are failing me.

Acknowledging my own part in this up-front, my observation is that we've spent a lot of letters on a semantic matter (by the dictionary definition of the term), without finding a clearly satisfying alternative. Maybe we can leave it as it is until that alternative is found in a future moment of genius, and make it a hidden gem in a future PR. ;)

I don't own any of these, so I'm not attuned to their zen. My angle is that I2S is a feature of the SOC, not really of the board it's on.

I own some, but not the M5StickC Plus2. My understanding from the related Discussion and PR comments is that everything was honky-dory until that device showed up and everything ran of the rails with the former I2S-based implementation.

But again, I do use the word "understanding", and I could be mistaken.

I'd like to understand why the M5's are special.

That's a fair question, and I'm not sure we now know. I for sure don't.

The less board special casing we have and the fewer exernal libraries we rely on, the happier I think we'll all be. That's one thing we pretty regularly agree on.

We still do... as long as it works.

— Reply to this email directly, view it on GitHub < https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/646#issuecomment-2254097401>,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/ACCSD37TOEPCXNKHJEEDQ4DZONU27AVCNFSM6AAAAABLRHJYIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGA4TONBQGE>

. You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub < https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/646#issuecomment-2254235850>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AA4HCFY764DRV6DJ4RJLD5TZOPZDJAVCNFSM6AAAAABLRHJYIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGIZTKOBVGA>.

You are receiving this because you were mentioned.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/646#issuecomment-2254246075, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD373BCI6RYSVMTQIPSTZOP66DAVCNFSM6AAAAABLRHJYIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGI2DMMBXGU . You are receiving this because you were mentioned.Message ID: @.***>

davepl commented 3 months ago

I doubt you can make it work with i2s across all models and have it be LESS complicated than it is now. Remember that one USE_M5 single case covers the M5 Stick C, Stick C Plus, Stick C Plus 2, Stack, Core 2, and so on because it uses the unified library. There are differences (including but not limited to different mics, I think) across those modules that the Unified library abstracts away.

If you could get it to work on all those models with one or two I2S config cases, I’d be good with that. I just think it’s more complicated than that.

Tale a look at MicClass.cpp in the M5 Unified library. There’s a lot of goo in there that special cases based on which module you have for pinouts, i2s channels, and so on. I’m afraid you’d wind up cluttering up our code with all those special cases if you support all the models.

On Jul 27, 2024, at 1:42 PM, Robert Lipe @.***> wrote:

The question, again, Is why there's a special case at all. There's a lovely i2s abstraction already in esp-idf/arduino that's used by everything else. The m5s we care about use esp32, so why not just talk to the esp32 instead of bouncing through a vendor library.

Other devices might use that same i2s mic if it's actually the mic that's being specially cased.

On Sat, Jul 27, 2024, 3:26 PM David W Plummer @.***> wrote:

Not sure I understand the question - it’s fixed. It works. No special cases, all M5 hardware uses the same code.

What are we trying to solve?

  • Dave

On Jul 27, 2024, at 12:36 PM, Robert Lipe @.***> wrote:

Let's at least change the spelling to the right word while this line is hot in our cerebral caches.

Id rather not buy one for study, but if that's what it takes, I can do so. It sure looks like that a plain ole ESP32-PICO-V3 module, so the i2s hardware should be the same as all the esp32-nothings.

If we have an issue with spm1423, we should have it on any board paired with that mic, not special cases for m5stick of the day. That's what https://docs.m5stack.com/en/core/M5StickC%20PLUS2 says that board is using, but it's also a little eval board that easy to attach to other boards.

If possible, I'd like to understand this at the electronics level and not marketing/convenience library level.

On Sat, Jul 27, 2024, 4:53 AM Rutger van Bergen @.***> wrote:

It's actually not your OWN language, which makes it even better!

Ha! :)

I could get behind that. Magnifier, Multiplier, amplitude, bias, boost, etc. are all in that basic basket but none of those seem quite right. It does seem like there's a better word for that, but at this hour, my own (native) language skills are failing me.

Acknowledging my own part in this up-front, my observation is that we've spent a lot of letters on a semantic matter (by the dictionary definition of the term), without finding a clearly satisfying alternative. Maybe we can leave it as it is until that alternative is found in a future moment of genius, and make it a hidden gem in a future PR. ;)

I don't own any of these, so I'm not attuned to their zen. My angle is that I2S is a feature of the SOC, not really of the board it's on.

I own some, but not the M5StickC Plus2. My understanding from the related Discussion and PR comments is that everything was honky-dory until that device showed up and everything ran of the rails with the former I2S-based implementation.

But again, I do use the word "understanding", and I could be mistaken.

I'd like to understand why the M5's are special.

That's a fair question, and I'm not sure we now know. I for sure don't.

The less board special casing we have and the fewer exernal libraries we rely on, the happier I think we'll all be. That's one thing we pretty regularly agree on.

We still do... as long as it works.

— Reply to this email directly, view it on GitHub < https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/646#issuecomment-2254097401>,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/ACCSD37TOEPCXNKHJEEDQ4DZONU27AVCNFSM6AAAAABLRHJYIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGA4TONBQGE>

. You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub < https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/646#issuecomment-2254235850>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AA4HCFY764DRV6DJ4RJLD5TZOPZDJAVCNFSM6AAAAABLRHJYIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGIZTKOBVGA>.

You are receiving this because you were mentioned.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/646#issuecomment-2254246075, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD373BCI6RYSVMTQIPSTZOP66DAVCNFSM6AAAAABLRHJYIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGI2DMMBXGU . You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/646#issuecomment-2254249643, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4HCF3MHS6BUVMXJL4FCTTZOQA23AVCNFSM6AAAAABLRHJYIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGI2DSNRUGM. You are receiving this because you were mentioned.

rbergen commented 3 months ago

I've also reviewed the latest changes, and will "approve by merging".