Fixed point overflow in init

[philippe44]

I think there is a big fat mistake in the fixed point init when user provides the buffer for sin/cos value with a lenght that is less than the compile-time default max fft len.

In addition, if you look into dsps_dotprod.h

#ifdef CONFIG_DSP_OPTIMIZED
#define dsps_dotprod_s16 dsps_dotprod_s16_ae32
#define dsps_dotprod_f32 dsps_dotprod_f32_ae32
#define dsps_dotprode_f32 dsps_dotprode_f32_ae32
#endif
#ifdef CONFIG_DSP_ANSI
#define dsps_dotprod_s16 dsps_dotprod_s16_ansi
#define dsps_dotprod_f32 dsps_dotprod_f32_ansi
#define dsps_dotprode_f32 dsps_dotprode_f32_ansi
#endif

Then DSP_ANSI and DSP_OPTIMIZED are not exclusive, but in Kconfig, you have

config DSP_ANSI
   bool "ANSI C"
config DSP_OPTIMIZED
   bool "ESP32 Optimized"
endchoice

config DSP_OPTIMIZATION
   int
   default 0 if DSP_ANSI
   default 1 if DSP_OPTIMIZED

Which means that DSP_ANSI is always defined so you end up in dsps_dotprod_s16 always pointing to the ansi version :-(

[dmitry1945]

Hi Philippe, Thank you for report. With buffer overflow problem is clear. I we have fix it.

But with DSP_OPTIMIZATION is not clear. I have check and everything works correct. For DSP_OPTIMIZED version it's compiled _ae32 version, for DSP_ANSI - _ansi.

Thanks, Dmitry

[philippe44]

Hi Dmitry - If you look at KConfig in the esp-dsp package, the DSP_ANSI and DSP_OPTIMIZED are both defined. I'm not a KConfig expert but I think indentation matters and the way this file is build, then DSP_ANSI and DSP_OPTIMIZED are always defined. They are not set, but then if you look at your header, when both are defined, then DSP_ANSI takes over I've verified that with that syntax

    choice DSP_OPTIMIZATION
        prompt "DSP Optimization for ESP32"
        default DSP_OPTIMIZED
        help
            An Ansi C version could be used for verification and debug purpose.
        config DSP_ANSI
            bool "ANSI C"
        config DSP_OPTIMIZED
            bool "ESP32 Optimized"
    endchoice

Then only one can be defined

[philippe44]

oh, and I've also seen that in the modules/test, the benchmark are at the wrong place which gives awesome results, hélas :-)

[igrr]

I'm not a KConfig expert but I think indentation matters and the way this file is build, then DSP_ANSI and DSP_OPTIMIZED are always defined.

Indentation in Kconfig files does not matter. If you have an example (i.e. reproducible case) where it does matter, please do post it! In this specific case the definition looks okay; I think if both options were somehow defined, C preprocessor would have given a redefinition warning. How do you determine that both options are defined in the sdkconfig.h file? If they are actually both #define-d in sdkconfig.h, can you please attach your sdkconfig, sdkconfig.h, and mention the IDF version?

[philippe44]

Indentation in Kconfig files does not matter. If you have an example (i.e. reproducible case) where it does matter, please do post it! In this specific case the definition looks okay; I think if both options were somehow defined, C preprocessor would have given a redefinition warning. How do you determine that both options are defined in the sdkconfig.h file? If they are actually both #define-d in sdkconfig.h, can you please attach your sdkconfig, sdkconfig.h, and mention the IDF version?

Yes, sorry this one is on me. I was having so many issues a couple of days ago to understand what was going on (the overflow was crashing my main large application and it was providing insanely slow results on an isolated test application - like 2M cycles for 1024 points -, but not crashing) that I mixed up things & builds. I agree compiler would have whined about redefinition.

[dmitry1945]

Hi Philippe,

I will publish changes soon. I have took your commit with buffer overflow. It will be in master. The changes for benchmarks is subject to discuss...

Thank you very much!

Regards, Dmitry

[philippe44]

I saw you changed float as well, so I'll close the patch