espressif / esp-dsp

DSP library for ESP-IDF
Apache License 2.0
442 stars 87 forks source link

FIR Convolution algorithm error (DSP-120) #77

Closed Davidliujx closed 8 months ago

Davidliujx commented 8 months ago

FIR in the esp-dsp lib :
esp_err_t dsps_fir_f32_ansi(fir_f32_t fir, const float input, float *output, int len) { for (int i = 0 ; i < len ; i++) { float acc = 0; int coeff_pos = 0; //int coeff_pos = fir->N - 1; fir->delay[fir->pos] = input[i]; fir->pos++; if (fir->pos >= fir->N) { fir->pos = 0; } for (int n = fir->pos; n < fir->N ; n++) { acc += fir->coeffs[coeff_pos++] * fir->delay[n];* //acc += fir->coeffs[coeff_pos--] fir->delay[n]; } for (int n = 0; n < fir->pos ; n++) { acc += fir->coeffs[coeff_pos++] * fir->delay[n];* //acc += fir->coeffs[coeff_pos--] fir->delay[n]; } output[i] = acc; } return ESP_OK; }

dmitry1945 commented 8 months ago

Hi @Davidliujx, We've changed the fir->coeffs[coeff_pos--] to the fir->coeffs[coeff_pos++] for speed optimization reason. The implementation In this way is just faster. But, for this, the order of your coefficients should be reversed. I will add the comment to the header about coefficients order. Thank you, Dmitry

TD-er commented 8 months ago

If you're really into speed optimizations, then you may also want to look at lines like these:

for (int n = fir->pos; n < fir->N ; n++) {

Using ++n in a loop is faster. Make as much as possible const in the invariant.

Thus:

const int fir_N = fir->N;
for (int n = fir->pos; n < fir_N; ++n) {
dmitry1945 commented 8 months ago

@TD-er yes, will add this to the list (it's not only FIR feature).

Thank you, Dmitry

Davidliujx commented 8 months ago

thanks for your all reply