search

blazoncek / WLED

Control WS2812B and many more types of digital RGB LEDs with an ESP8266 or ESP32 over WiFi!
MIT License
31 stars 0 forks source link

soundreactive: fftResult should be 16bit int type #31

Closed softhack007 closed 2 years ago

softhack007 commented 2 years ago

This is a reminder to change the following data type to int16_t, as discussed today.

https://github.com/blazoncek/WLED/blob/dbe90eb3f53b34876bf99fa9b311715c8fbd5067/usermods/audioreactive/audio_reactive.h#L94

https://github.com/blazoncek/WLED/blob/dbe90eb3f53b34876bf99fa9b311715c8fbd5067/usermods/audioreactive/audio_reactive.h#L782-L783

Currently the data for fftResult is constrained to [0...254] however this is not a very good solution. The real fftresult may have a completely different magnitude, and the code we have today simply cuts away everything above 254. In future we will probably change this.

So it would be better now - as we adopt many effects to the new framework - to already use uint16_t instead of an 8bit type.

blazoncek commented 2 years ago

Thinking of it, fftResult[] is an end product used for calculations within WLED effect function (not related to audio processing). As such (combined with matrices where those values may be used most often) it may be an overkill to have those values use 16 bits (instead of 8) doubling the required amount of RAM. On ESP32 it may not matter much, but needlesly wasting memory on ESP8266 may become painful over time.

Let's ask this: Do we really need values >255 for use in effect function?

If the answer is not absolutely then maybe think twice before deciding on 16 bit. I do agree that with greater resolution we may do more things (or easier) but is it really neccessary?

softhack007 commented 2 years ago

Let's ask this: Do we really need values >255 for use in effect function?

Yes that's a point. Currently all effect functions assume that fftResult[] values are <= 255. It's a pity because a lot of things would be possible with 16 bit, but then the best solution would be float which really preserves accuracy. Maybe let's wait what comes. Internally, we already have accurate results in static float fftCalc[16].

So yes, agreed. Until some effect wants the real fftresult (without saturation at 255), we can keep it as-is.

If you want, we can close this issue.

blazoncek commented 2 years ago

Then let's add fftCalc[] as a data exchange option. The downside may be that effect could change content…

softhack007 commented 2 years ago

👍 good idea.

blazoncek commented 2 years ago

We will add float fftCalc[] into usermod exchange data structure as it only takes 5 additional bytes (pointer + type).