spectrum samples: ensure powers of two #832
Conversation
**Explanation of Changes Made (in the working modification):** This core change was to ensure that data segments fed into Fast Fourier Transform (FFT) operations have lengths that are powers of two. This was achieved by: 1. Introducing a helper function `_getLargestPowerOfTwoLeq(n)` to find the largest power of two less than or equal to a given number `n`. 2. Applying this function to determine the `effectiveSampleCount` in `dataLoadFrequency`, the `pointsPerSegment` in `dataLoadPSD`, and the `fftChunkLength` in `_dataLoadFrequencyVsX`. 3. Adding a `MIN_FFT_POINTS` constant and checks to prevent FFTs on overly small data segments, returning empty/default results if necessary. 4. Adjusting the initialization and processing of `matrixFftOutput` in `_dataLoadFrequencyVsX` to correctly handle the (potentially modified) power-of-2 `fftChunkLength`, especially concerning the dimensions of the output matrix and the averaging loop. The original logic of slicing the complex FFT output and processing its first `fftChunkLength` elements was maintained, but now `fftChunkLength` is a power of two. **Overall Function & Impact of Changes:** `GraphSpectrumCalc` is an object responsible for performing various signal processing calculations on flight log data, primarily to analyze frequency spectrums and noise characteristics. Its key functions include: * Calculating standard FFTs on selected data channels (`dataLoadFrequency`). * Computing Power Spectral Density (PSD) using Welch's method (`dataLoadPSD`). * Generating frequency-vs-throttle or frequency-vs-RPM heatmaps by performing FFTs on short, overlapping time chunks of data and correlating them with throttle/RPM values (`_dataLoadFrequencyVsX`). * Analyzing PID error relative to setpoint values (`dataLoadPidErrorVsSetpoint`). This commit specifically ensures FFT input lengths are powers of two, primarily impact the **performance and potentially the numerical stability/accuracy** of the FFT calculations within these functions. FFT algorithms are often highly optimized for power-of-two input sizes. By adhering to this, the browser's JavaScript engine or the underlying FFT library (`FFT.js`) can execute these computations much more efficiently, reducing processing time and potentially alleviating UI stalls or slowdowns previously experienced when analyzing large datasets or performing many FFTs (as in the heatmap generation). The introduction of `MIN_FFT_POINTS` also adds robustness by preventing calculations on impractically small data segments. The core analytical goals remain the same, but they are now achieved with potentially improved computational speed.
WalkthroughThe update enforces a minimum FFT size of 32 points and ensures all FFT and PSD calculations use the largest power-of-two length not exceeding the available sample count. Functions are revised to skip spectral analysis if insufficient data is present, and logic is adjusted for correct handling of FFT output, PSD scaling, and edge cases in spectral analysis. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GraphSpectrumCalc
User->>GraphSpectrumCalc: Request spectral analysis (FFT/PSD)
GraphSpectrumCalc->>GraphSpectrumCalc: Check sample count
alt Sample count >= MIN_FFT_POINTS
GraphSpectrumCalc->>GraphSpectrumCalc: Find largest power-of-two <= sample count
GraphSpectrumCalc->>GraphSpectrumCalc: Truncate/slice samples
GraphSpectrumCalc->>GraphSpectrumCalc: Perform FFT/PSD calculations
GraphSpectrumCalc->>User: Return spectral result
else Sample count < MIN_FFT_POINTS
GraphSpectrumCalc->>User: Return empty result with warning
end
Possibly related PRs
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@demvlad , @haslinghuis . i still feel a next step would be memory optimization for avoiding samples-data copies. (unless i'm mistaken by way of A.I. reports) |
prompt:
A.I. :Yes, I can confirm that array slicing (
So, the strategy involves:
The current code does retain the use of |
prompt:A.I.:Let's break down those specific changes: 1. Why was
2. Why was
3. Why was
|
|
Ok. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/graph_spectrum_calc.js (1)
203-217: 🛠️ Refactor suggestionPre-allocate rows with correct length and drop emergency re-allocation
matrixFftOutputis initialised withfftChunkLength * 2columns, but only the firstfftChunkLengthvalues are ever written afterfftOutput.slice(0, fftChunkLength).
Later you createfinalMatrixFftOutputwith the right width and copy data over, doubling memory and work.-const matrixFftOutput = new Array(NUM_VS_BINS).fill(null).map(() => new Float64Array(fftChunkLength * 2)); +const matrixFftOutput = new Array(NUM_VS_BINS).fill(null).map(() => new Float64Array(fftChunkLength)); ... -// Safety, matching slice -if (!matrixFftOutput[vsBinIndex]) matrixFftOutput[vsBinIndex] = new Float64Array(fftChunkLength); matrixFftOutput[vsBinIndex][i] += fftOutput[i];With the correct width from the start, the “safety” check and the whole
finalMatrixFftOutputcopy/loop (lines 260-279) can be deleted—saving ~2× RAM and CPU for every heat-map render.
🧹 Nitpick comments (3)
src/graph_spectrum_calc.js (3)
40-47: Replace brute‐force loop with bit-wise math for O(1) power-of-two computation
_getLargestPowerOfTwoLeqwalks one bit at a time which is fine for small inputs but unnecessarily slow for largen.
A branch-less bit-twiddle is simpler and constant-time:-_getLargestPowerOfTwoLeq: function(n) { - if (n < 1) return 0; - let p = 1; - while ((p * 2) <= n) { - p *= 2; - } - return p; -}, +_getLargestPowerOfTwoLeq(n) { + /* Using 32-bit trick – works while n < 2³¹, plenty for FFT sizes */ + if (n < 1) return 0; + n |= n >> 1; + n |= n >> 2; + n |= n >> 4; + n |= n >> 8; + n |= n >> 16; + return (n + 1) >>> 1; +},This keeps semantics identical but removes the loop.
388-410: Avoid returning a huge zero-filled array
_getFlightSamplesFreqallocatesMAX_ANALYSER_LENGTH / …samples (~millions) irrespective of actual log size and returns it unsliced.
Callers must slice every time, wasting memory and GC.Consider:
-const samples = new Float64Array(MAX_ANALYSER_LENGTH / (1000 * 1000) * this._blackBoxRate); +const samples = []; ... -samples[samplesCount] = value; +samples.push(value); ... -return { samples, count: samplesCount }; +return { samples: Float64Array.from(samples), count: samplesCount };Typed-array creation is postponed until the exact size is known, drastically lowering peak memory.
653-657: Nit: repeatedif/elseladder could be expressed with a lookupMinor readability tweak – a tiny table would avoid the nested conditionals here, but functionality is correct.
This comment was marked as outdated.
This comment was marked as outdated.
|
... have we someone as expert by fft? :) |
nerdCopter
force-pushed
the
20250613_AI_ensure_poweres_of_two
branch
from
6f63efb to
f12397e
Compare
|
why does this PR output spectrums that are different than master's output? this PR: master: |
|
Tip For best results, initiate chat on the files or code changes. @nerdCopter The differences in the spectrum outputs are expected and are a direct result of the improvements made in this PR. Here are the main reasons:
These changes should make the spectral analysis more mathematically correct and computationally efficient, even though the visual output differs from the previous version. The new version is showing a more accurate representation of the frequency content with better numerical properties and a wider dynamic range. |
|
@KarateBrot , @Quick-Flash , @ledvinap , @demvlad , thoughts about these changes specifically in reference to FFT and coderrabbitAI's review? This PR was essentially a fix to #819 breakage (which was reverted). I used Google A.I. studio to produce code, then coderrabbitAI suggested a fix here. I am no mathematician and cannot discern validity personally. |
|
The source code spectrum charts view by freq axis: The source spectrum charts has huge values count, what are looks like true. But this is wrong math IMO. |
Well you are using a power of 2 FFT, which is faster, but now it doesn't use the entire log to generate this as some portion of the flight needs to be cut off. At worst almost half the flight could be cut off. |
|
Tip For best results, initiate chat on the files or code changes. @Quick-Flash That's correct! You've identified one of the key reasons for the visual differences. By enforcing power-of-2 sizes for the FFT input, we're improving computational efficiency and numerical stability, but potentially at the cost of discarding some data. In the worst case, just under 50% of samples could be trimmed to fit the power-of-2 constraint. For example, if there are 65,000 samples, we'd use 32,768 (2^15) samples and discard 32,232. For future improvements, we could consider alternatives like:
These approaches would maintain the computational benefits while potentially utilizing more of the input data. Would you be interested in exploring any of these alternatives in a future PR? ✏️ Learnings added
|
|
@coderabbitai , please propose code to implement "Zero-padding the data to the next power of 2 instead of truncating" as you previously mentioned. |
|
Tip For best results, initiate chat on the files or code changes. |
preview does NOT MATCH the current code state. |
|
local "codeRabbit" in vscode reports this in current state: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/graph_spectrum_calc.js (3)
40-56: UseMath.pow(2, ⌊log₂(n)⌋)instead of a loop for power-of-two helpersBoth
_getLargestPowerOfTwoLeqand_getNextPowerOfTwowalk up the powers of two with awhileloop.
For largen(e.g. > 10 M samples) these helpers may be called thousands of times inside tight UI code and introduce avoidable CPU overhead.-_getLargestPowerOfTwoLeq: function(n) { - if (n < 1) return 0; - let p = 1; - while ((p * 2) <= n) { - p *= 2; - } - return p; -}, +_getLargestPowerOfTwoLeq(n) { + return n < 1 ? 0 : 2 ** Math.floor(Math.log2(n)); +}, -_getNextPowerOfTwo: function(n) { - if (n <= 0) return 1; - let p = 1; - while (p < n) { - p *= 2; - } - return p; -}, +_getNextPowerOfTwo(n) { + return n <= 1 ? 1 : 2 ** Math.ceil(Math.log2(n)); +},This removes the loop, keeps behaviour identical, and is easier to read.
(Fallback to the loop only ifMath.log2is unavailable in a legacy target.)
115-134: Avoid double copying of large sample buffers when zero-padding
flightSamples.samples.slice(0, flightSamples.count)allocates and copies up to 5 min of data, thenFloat64Array.setcopies it once more intosamplesForFft.You can cut the memory-bandwidth in half by using a
subarrayview:-const samplesForFft = new Float64Array(targetFftLength); -samplesForFft.set(flightSamples.samples.slice(0, flightSamples.count)); +const samplesForFft = new Float64Array(targetFftLength); +samplesForFft.set(flightSamples.samples.subarray(0, flightSamples.count));
subarrayis zero-cost because it shares the backing buffer, andsetperforms the only required copy into the zero-padded block.
645-662: Hanning-window scale: building a length-pointsPerSegmentJS array every call is wasteful
const window = Array(pointsPerSegment).fill(1); this._hanningWindow(window, pointsPerSegment);creates and initialises an O(N) array only to square or sum its elements once.
For 8 × over-segmented PSD this dominates runtime.Consider computing the closed-form constants instead:
// analytical values for a Hann window const hannPowSum = pointsPerSegment * 3/8; // Σ w[n]² const hannLinSum = pointsPerSegment * 1/2; // Σ w[n]or cache the prepared window & its sums by
pointsPerSegmentin aMap.This avoids repeated large allocations during interactive zoom.
|
PR at 6f6d279 on left. (zero-filled to next 2^N). master on right: prompt to Google AI Studio:
AI:Why the Spectrums Look Different:
In summary: The primary reason the spectrums are different is that the original code (right image) calculates the "spectrum" values incorrectly by taking While both methods involve zero-padding (your new method explicitly to the next power of two, the old method implicitly to a very large buffer size), the mathematical error in processing the FFT output in the old code is the most significant cause of the visual discrepancy and the likely incorrectness of the right-hand spectrum. |
|
Preview URL: https://d94d11a7.betaflight-blackbox.pages.dev |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/graph_spectrum_calc.js (1)
49-56: Implement edge case handling in _getNextPowerOfTwo.The implementation correctly finds the next power of two greater than or equal to a given number, but the edge case handling could be improved for clarity.
- if (n <= 0) return 1; // Return 1 for non-positive inputs, MIN_FFT_POINTS check will handle later. + if (n <= 0) return 1; // Return smallest power of 2 for non-positive inputs, MIN_FFT_POINTS check will handle minimum size later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/graph_spectrum_calc.js(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Quick-Flash
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-05-13T17:48:20.403Z
Learning: When using FFT (Fast Fourier Transform), enforcing power-of-2 input lengths improves computational efficiency and numerical stability but can result in discarding up to almost 50% of the flight data in worst-case scenarios.
🔇 Additional comments (19)
src/graph_spectrum_calc.js (19)
21-22: Good addition of MIN_FFT_POINTS constant.Establishing a minimum threshold of 32 points for FFT calculations is a good practice to ensure meaningful spectral analysis. This directly addresses the PR objective of ensuring proper FFT processing.
40-47: Well-implemented power-of-two utility function.This helper function correctly finds the largest power of two less than or equal to a given number, which is essential for proper FFT length calculation. The implementation handles edge cases appropriately (n < 1).
115-117: Good implementation of zero-padding to next power of two.Using
_getNextPowerOfTwofor the FFT input length ensures optimal FFT performance while the zero-padding approach preserves all the original data.
118-128: Robust error handling for insufficient data.The check for minimum FFT points and zero samples is a good defensive programming practice. This will prevent attempting FFT on meaningless data and potential numerical instabilities.
130-133: Efficient implementation of zero-padding.Creating a new array of the target length and using
set()to copy only the actual samples is an efficient approach to zero-padding.
150-194: Well-implemented Power Spectral Density calculation.The new
dataLoadPSDmethod correctly implements Welch's method for PSD estimation with appropriate segment sizing, overlap, and error handling. The dynamic adjustment of segment size based on zoom level is a nice feature for interactive analysis.A few implementation details to note:
- Properly ensures that segment length is a power of two using
_getLargestPowerOfTwoLeq- Correctly skips PSD calculation when data is insufficient
- Uses standard 50% overlap between segments
- Returns complete metadata including min/max values and max noise frequency
201-204: Proper power-of-two sizing for frequency-vs-X processing.The implementation correctly determines the desired samples per chunk and then rounds up to the next power of two, which optimizes FFT performance.
206-217: Good error handling for insufficient chunk size.Skipping FreqVsX processing when the chunk length is too small is appropriate and prevents meaningless results.
248-257: Correctly implemented FFT magnitude calculation.This implementation properly computes the magnitude of complex FFT outputs using
Math.hypot(re, im), fixing the issue mentioned in the past review comments where individual real/imaginary components were incorrectly treated as separate magnitude values.
271-276: Robust bounds checking for bin indexing.The additional check
if (vsBinIndex >=0 && vsBinIndex < NUM_VS_BINS)prevents potential out-of-bounds access, which is a good defensive programming practice.
280-291: Improved handling of bins with no samples.The updated averaging logic correctly handles the case where a bin has no samples by maintaining an array of zeros, which avoids division by zero errors.
463-469: Good minimum segment size enforcement for VS averaging.Ensuring the segment for VS averaging is not smaller than MIN_FFT_POINTS is consistent with the rest of the implementation and helps maintain meaningful results.
490-502: Improved robustness for handling invalid ranges.The enhanced fallback logic for invalid VS value ranges with explicit conditions and better logging improves code robustness and debuggability.
505-506: Helpful documentation for oversized arrays.The comments clarifying that returned arrays are oversized help prevent bugs in caller code and improve maintainability.
581-620: Correct implementation of FFT magnitude calculation.The revised _normalizeFft method now properly computes magnitudes from complex FFT outputs using
Math.hypot(re, im)and handles the correct number of unique frequency bins (N/2+1). This directly addresses the issue raised in previous reviews. The improved documentation and safeguards against division by zero also enhance code robustness.The implementation:
- Correctly calculates the number of unique frequency bins (N/2+1)
- Properly computes magnitude from complex pairs
- Accurately scales bin index to frequency
- Includes detailed comments explaining the scaling calculations
622-630: Proper return structure for FFT data.The return structure now correctly includes the magnitude spectrum and the proper number of frequency bins, fixing issues from the previous implementation.
632-723: Well-implemented Power Spectral Density calculation method.The new _psd method implements Welch's method correctly, with proper scaling for different window types, appropriate handling of one-sided PSD doubling, and good robustness against edge cases.
Key strengths:
- Proper scaling factors for windowed and non-windowed data
- Correct doubling for one-sided PSD (except DC and Nyquist)
- Good handling of empty inputs
- Appropriate frequency indexing for noise detection
The change of minimum value from 1e-7 to 1e-10 expands the dynamic range from approximately -70dB to -100dB as mentioned in the PR objectives, allowing visualization of weaker signals.
726-766: Robust implementation of segmented FFT processing.The new _fft_segmented method correctly implements FFT processing for overlapping segments, with good parameter validation and proper handling of the Hanning window. The implementation correctly computes magnitudes from complex FFT output pairs.
Notable features:
- Early returns for invalid parameters (segment length of zero, below MIN_FFT_POINTS, or larger than sample count)
- Proper handling of overlap between segments
- Correct computation of magnitudes from complex FFT outputs
- Good warning messages when parameters would cause issues
136-138:✅ Verification successful
Verify Hanning window application on zero-padded data.
Applying the Hanning window to the entire zero-padded array might not be ideal as it will affect the padded zeros unnecessarily. However, since the zeros will remain zeros after windowing, this approach is functionally correct.
🏁 Script executed:
#!/bin/bash # Check if this implementation is consistent with the rest of the codebase # by looking for other instances of Hanning window application echo "Searching for other implementations of Hanning window application in the codebase..." rg "hanningWindow|hanning|window" -A 3 -B 3Length of output: 65675
No action needed: Hanning window on zero-padded FFT input is safe
We verified that this is the only use of
_hanningWindowin the codebase and multiplying the zero-padded tail by the window coefficients still yields zeros. The implementation is consistent and functionally correct—no changes required.
|
@coderabbitai , please perform full in-depth review and analysis. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 28217 @nerdCopter Here's my comprehensive in-depth review and analysis of the Core Improvements
Technical AnalysisThe code properly handles the full FFT pipeline:
Performance Considerations
Visual Differences ExplainedThe changes in spectrum appearance compared to master are expected and represent more accurate results due to:
Future Optimization OpportunitiesAs you mentioned, memory optimization would be a logical next step:
Overall, this PR significantly improves both the mathematical correctness and computational efficiency of the spectral analysis in Blackbox Log Viewer. The code now follows standard DSP practices and should provide more reliable and accurate results. 🐰💡 |
|
@nerdCopter you could always just center your data in a power of 2 size that is larger than your data, then take the data and mirror the ends to fill in the gaps (you still probably want to do some hanning or something). This is one method, but you do still get some distortions. If power of 2 is much faster, you could possibly just do one run of the data that covers the first data you get and another that just covers the end. Then do some sort of averaging of the two (still not perfect, but there is no perfect answer if you want to keep the FFT to a power of 2). |
Explanation of Changes Made (in the working modification):
This core change was to ensure that data segments fed into Fast Fourier Transform (FFT) operations have lengths that are powers of two. This was achieved by:
_getLargestPowerOfTwoLeq(n)to find the largest power of two less than or equal to a given numbern.effectiveSampleCountindataLoadFrequency, thepointsPerSegmentindataLoadPSD, and thefftChunkLengthin_dataLoadFrequencyVsX.MIN_FFT_POINTSconstant and checks to prevent FFTs on overly small data segments, returning empty/default results if necessary.matrixFftOutputin_dataLoadFrequencyVsXto correctly handle the (potentially modified) power-of-2fftChunkLength, especially concerning the dimensions of the output matrix and the averaging loop. The original logic of slicing the complex FFT output and processing its firstfftChunkLengthelements was maintained, but nowfftChunkLengthis a power of two.Overall Function & Impact of Changes:
GraphSpectrumCalcis an object responsible for performing various signal processing calculations on flight log data, primarily to analyze frequency spectrums and noise characteristics. Its key functions include:dataLoadFrequency).dataLoadPSD)._dataLoadFrequencyVsX).dataLoadPidErrorVsSetpoint).This commit specifically ensures FFT input lengths are powers of two, primarily impact the performance and potentially the numerical stability/accuracy of the FFT calculations within these functions. FFT algorithms are often highly optimized for power-of-two input sizes. By adhering to this, the browser's JavaScript engine or the underlying FFT library (
FFT.js) can execute these computations much more efficiently, reducing processing time and potentially alleviating UI stalls or slowdowns previously experienced when analyzing large datasets or performing many FFTs (as in the heatmap generation). The introduction ofMIN_FFT_POINTSalso adds robustness by preventing calculations on impractically small data segments. The core analytical goals remain the same, but they are now achieved with potentially improved computational speed.EDIT: additional fixes and zero-filling to next 2^N as per AI consultation.
EDIT: this re-introduces slicing as intended by @demvlad 's #819
Summary by CodeRabbit
Bug Fixes
New Features
Improvements