Skip to content

Conversation

@demvlad
Copy link
Contributor

@demvlad demvlad commented Apr 28, 2025

Resolved issue of missing slices of data samples arrays in spectrum calc.
The FFT used huge rezerved data length input before.

@demvlad demvlad force-pushed the slice_spectrum_samples branch from 8f664d7 to ef22d74 Compare April 28, 2025 14:49
@sonarqubecloud
Copy link

@nerdCopter
Copy link
Member

we dont have Netlify previews at all anymore @blckmn ?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes an issue with the FFT calculation by correcting the sample slicing for spectrum processing. Key changes include:

  • Truncating the samples array in _getFlightSamplesFreq to use only populated data.
  • Applying slicing to vsValues arrays in _getFlightSamplesFreqVsX.
  • Slicing the piderror and setpoint arrays in _getFlightSamplesPidErrorVsSetpoint to match the actual sample count.
Comments suppressed due to low confidence (3)

src/graph_spectrum_calc.js:302

  • Confirm that slicing using samplesCount does not exceed the allocated array length and that samplesCount is properly maintained.
    samples : samples.slice(0, samplesCount),

src/graph_spectrum_calc.js:418

  • Verify that slicing the 'piderror' TypedArray to samplesCount preserves the intended data and aligns with downstream FFT processing.
    piderror: piderror.slice(0, samplesCount),

src/graph_spectrum_calc.js:419

  • Ensure the sliced 'setpoint' array correctly reflects the populated data count and remains in sync with the corresponding 'piderror' values.
    setpoint: setpoint.slice(0, samplesCount),

@haslinghuis haslinghuis merged commit 3639e69 into betaflight:master Apr 29, 2025
2 checks passed
Copy link
Member

@nerdCopter nerdCopter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • approving untested.

@demvlad
Copy link
Contributor Author

demvlad commented Apr 30, 2025

It was not big issue as i've thought on begin :). The FFT get real data count as parameter. But it allocate float data memory for huge data count, what is not good.

@demvlad demvlad deleted the slice_spectrum_samples branch April 30, 2025 16:29
@nerdCopter
Copy link
Member

i should not have approved untested.

several reports of breakage.
tested master, this PR has broken "spectrum graphs"

@haslinghuis
Copy link
Member

haslinghuis commented May 12, 2025

@demvlad please have a look at #828

@demvlad
Copy link
Contributor Author

demvlad commented May 13, 2025

@haslinghuis @nerdCopter
I doubted that it was bug and i've found an answer.
The source spectrum code used FFT zero padding method, to get bigger fft resolution at the chart. It uses big input/output size with big zero values pad. As result we had high fft resolution as fs/N. But zero pad does not change frequency resolution, what is 1/T.
I want try to use fft input size as nearest power 2 value and fill zero values in empty cells only.
The both methods is working. But we need to test and compare new method before.
Those methods choice is ground of my next PSD PRs. I will need the big rebase of PSD PRs in any case. I will use new method for PSD.
Therefore i suggest to revert this and PSD :) PRs.
The PSD works good and i have power2 sized version, but let's test and compare the all these before.

@demvlad demvlad restored the slice_spectrum_samples branch May 13, 2025 11:41
demvlad added a commit to demvlad/blackbox-log-viewer that referenced this pull request May 13, 2025
haslinghuis pushed a commit that referenced this pull request May 13, 2025
…#831)

Revert "Added missing slices of data samples arrays in spectrum calc (#819)"

This reverts commit 3639e69.
@demvlad demvlad deleted the slice_spectrum_samples branch September 17, 2025 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants