The spectrum charts improvements. #833
Conversation
## Walkthrough
This change introduces comprehensive support for Power Spectral Density (PSD) analysis and visualization. It adds PSD computation using Welch's method, new PSD-related spectrum types, corresponding UI controls, and rendering logic. FFT buffer handling is standardized to power-of-two sizes, and magnitude calculations are corrected. The UI and CSS are updated to integrate PSD controls and visualizations.
## Changes
| File(s) | Change Summary |
|----------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| src/graph_spectrum_calc.js | Adds PSD computation via Welch’s method, new constants, PSD data loading methods, power-of-two FFT buffer sizing, corrected magnitude calculations, and clarifying comments. Exports and updates several methods and constants for PSD support. |
| src/graph_spectrum_plot.js | Adds new PSD spectrum types, constants, state variables, setters, and rendering methods for PSD graphs and heatmaps. Updates graph drawing, heatmap, grid, and interaction logic to support PSD and related UI changes. |
| src/graph_spectrum.js | Integrates PSD spectrum types and controls into the analyser UI. Updates data loading, plotting, and event handling for PSD. Refactors `dataBuffer` to local scope. Adds handlers for new PSD sliders and updates spectrum type selection logic for PSD UI elements. |
| index.html | Adds PSD spectrum type options to the dropdown. Introduces new input fields and labels for PSD controls in the analyser section. Adjusts option ordering and adds new UI elements for PSD min/max/limit. |
| src/css/main.css | Adds and adjusts styles for new PSD controls and labels in the analyser. Updates positioning and display rules for PSD-related UI elements. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant UI (index.html)
participant Analyser (graph_spectrum.js)
participant Calc (graph_spectrum_calc.js)
participant Plot (graph_spectrum_plot.js)
User->>UI (index.html): Selects PSD spectrum type / adjusts PSD sliders
UI->>Analyser: Triggers spectrum type or slider change event
Analyser->>Calc: Calls dataLoadPowerSpectralDensity* or dataLoadPSD
Calc->>Calc: Computes FFT, applies Welch method, calculates PSD
Calc-->>Analyser: Returns PSD data
Analyser->>Plot: Calls plotSpectrum with PSD data and parameters
Plot->>Plot: Renders PSD graph/heatmap with new min/max/low-level settings
Plot-->>UI: Updates canvas with PSD visualizationPossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/graph_spectrum_calc.js (2)
112-118: Nit — typo and clearer variable naming
multipileris miss-spelled in two places and the intent (“zoom multiplier”) is not immediately obvious. A single, well-named variable makes the block easier to read.-const multipiler = Math.floor(1 / analyserZoomY); // 0. ... 10 -if (multipiler == 0) { +const zoomMultiplier = Math.floor(1 / analyserZoomY); // 0 … 10 +if (zoomMultiplier === 0) { pointsPerSegment = 256; -} else if (multipiler > 1) { - pointsPerSegment *= 2 ** Math.floor(multipiler / 2); +} else if (zoomMultiplier > 1) { + pointsPerSegment *= 2 ** Math.floor(zoomMultiplier / 2); }Also applies to: 120-124
159-165: Heavy allocations inside the FFT loop hurt performanceA new
FFTinstance and two large typed arrays are created for every sliding
window. For a 5-minute trace with the default settings this can run into
thousands of allocations and GC pauses.Minimal fix (reuse across iterations):
-const fft = new FFT.complex(fftBufferSize, false); -for (let fftChunkIndex = 0; …) { - const fftInput = new Float64Array(fftBufferSize); - let fftOutput = new Float64Array(fftBufferSize * 2); +const fft = new FFT.complex(fftBufferSize, false); +const fftInput = new Float64Array(fftBufferSize); +const fftOutput = new Float64Array(fftBufferSize * 2); +for (let fftChunkIndex = 0; …) { … }The same input/output buffers can be safely overwritten each iteration, cutting
memory churn dramatically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/graph_spectrum_calc.js(9 hunks)
🔇 Additional comments (1)
src/graph_spectrum_calc.js (1)
347-353: Zero-padding check — please verify counts
samples.slice(0, fftBufferSize)returns a buffer that may be longer than the
actual data (samplesCount). Later, a Hann window is applied only to
samplesCountpoints, leaving the zero-padded tail untouched, which is fine.
Just make sure downstream code (plots, labels) interpretscountcorrectly
(i.e. as pre-pad length).
This comment was marked as outdated.
This comment was marked as outdated.
You are doing a huge job and made several steps forward compared to me. I was always using Python libraries for FFT without deep diving into the math. |
You have much drone tuning experience! These improvements change spectrum views, will very helpful to know what is good. |
demvlad
force-pushed
the
fft_input_size_issue
branch
from
ec62340 to
966e0cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/graph_spectrum_calc.js (3)
503-519:⚠️ Potential issueIncorrect FFT magnitude calculation in
_normalizeFft.Like the issue in the VS-X path, this function also calculates magnitude incorrectly by taking the absolute value of only the real part of each complex pair.
Implement the correct magnitude calculation as suggested in the previous review comments:
-const fftLength = fftOutput.length / 2; -// TODO: This is wrong spectrum magnitude calculation as abs of separate complex Re, Im values. -// We should use hypot(Re, Im) instead of and return divide by 2 (fftOutput.length / 4) arrays size -for (let i = 0; i < fftLength; i++) { - fftOutput[i] = Math.abs(fftOutput[i]); - if (i > noiseLowEndIdx && fftOutput[i] > maxNoise) { - maxNoise = fftOutput[i]; - maxNoiseIdx = i; - } -} +const complexPairs = fftOutput.length / 2; +const magnitudes = new Float64Array(complexPairs); +for (let i = 0; i < complexPairs; i++) { + const re = fftOutput[2 * i]; + const im = fftOutput[2 * i + 1]; + magnitudes[i] = Math.hypot(re, im); + if (i > noiseLowEndIdx && magnitudes[i] > maxNoise) { + maxNoise = magnitudes[i]; + maxNoiseIdx = i; + } +} +// Replace fftOutput with magnitudes for further processing +fftOutput = magnitudes;
178-186:⚠️ Potential issueIncorrect FFT magnitude calculation flagged by TODO.
The current implementation only uses the real part of each complex pair and applies
Math.abs()to that value alone, instead of computing the proper magnitude.This is exactly the issue mentioned in the previous review comments. You should implement the magnitude calculation using
Math.hypot(re, im)as already done in the new_fft_segmentedmethod:-fftOutput = fftOutput.slice(0, fftBufferSize); // The fft output contains two side spectrum, we use the first part only to get one side -// TODO: This is wrong spectrum magnitude calculation as abs of separate complex Re, Im values. -// We should use hypot(Re, Im) instead of and return divide by 2 (fftOutput.length / 4) arrays size -// Use only abs values -for (let i = 0; i < fftBufferSize; i++) { - fftOutput[i] = Math.abs(fftOutput[i]); - maxNoise = Math.max(fftOutput[i], maxNoise); -} +// Convert complex FFT output to magnitude spectrum +const magnitudes = new Float64Array(fftBufferSize / 2); +for (let i = 0; i < fftBufferSize / 2; i++) { + const re = fftOutput[2 * i]; + const im = fftOutput[2 * i + 1]; + magnitudes[i] = Math.hypot(re, im); + maxNoise = Math.max(magnitudes[i], maxNoise); +} +// Replace fftOutput with magnitudes for further processing +fftOutput = magnitudes;
222-230: 🛠️ Refactor suggestionUpdate matrix storage to use proper FFT magnitude size.
The
matrixFftOutputarray is sized tofftBufferSize * 2, which is twice as large as needed when using proper magnitude calculations.After implementing the correct magnitude calculation, you should reduce the matrix size:
const fftData = { fieldIndex : this._dataBuffer.fieldIndex, fieldName : this._dataBuffer.fieldName, - fftLength : fftBufferSize, - fftOutput : matrixFftOutput, + fftLength : fftBufferSize / 2, // One-sided spectrum length after magnitude calculation + fftOutput : matrixFftOutput, // This should also be updated to store magnitudes with half the size maxNoise : maxNoise, blackBoxRate : this._blackBoxRate, vsRange : { min: flightSamples.minValue, max: flightSamples.maxValue}, };
🧹 Nitpick comments (1)
src/graph_spectrum_calc.js (1)
162-169: Zero-padding implementation could be improved.The current implementation uses a loop to copy samples into a power-of-two sized buffer, with a TODO comment about finding a better resize method.
For better performance, consider using
TypedArray.set():-//TODO: to find method to just resize samples array to fftBufferSize -const samples = flightSamples.samples.slice(fftChunkIndex, fftChunkIndex + fftChunkLength); -for (let i = 0; i < fftChunkLength; i++) { - fftInput[i] = samples[i]; -} +// Efficiently copy samples to power-of-two sized buffer (zero-padding) +const samples = flightSamples.samples.slice(fftChunkIndex, fftChunkIndex + fftChunkLength); +fftInput.set(samples);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/graph_spectrum_calc.js(10 hunks)
🔇 Additional comments (9)
src/graph_spectrum_calc.js (9)
97-97: Good clarification in comment.The comment clarifies that the Hanning window is applied only to the actual sample count values, not the entire buffer.
104-104: Parameter removed from_normalizeFftcall.The explicit
fftLengthparameter has been removed, which is good as the function now calculates this value internally.
109-140: Well-implemented PSD calculation using Welch's method.The new
dataLoadPSDmethod is a valuable addition that:
- Dynamically calculates segment size based on zoom level
- Ensures segment size is a power of two for optimal FFT performance
- Sets appropriate overlap for Welch's method
- Returns structured output with min/max values and max noise frequency
This implementation follows best practices for spectral density estimation.
149-150: Good optimization using power-of-two FFT buffer size.Using a power-of-two size for FFT buffers is crucial for optimal FFT performance, as mentioned in the PR objectives. This change implements that optimization correctly.
197-197: Improved bin index calculation.Changing from
Math.floortoMath.roundprovides more accurate binning for frequency vs X charts.
328-351: Good optimization with power-of-two sample size.Adding the
scaledparameter and ensuring the returned samples are padded to a power-of-two length improves FFT performance. This is a well-implemented change.
465-468: Memory optimization by slicing arrays to actual sample count.This change ensures that only the necessary data is returned, which is more memory-efficient.
535-619: Excellent implementation of Power Spectral Density calculation.The new
_psdmethod correctly implements Welch's method for PSD calculation with:
- Proper window scaling for both 'density' and 'spectrum' output types
- Correct averaging of scaled power values
- Conversion to dB scale
- Tracking of min/max values and max noise frequency
This is a significant improvement for spectrum analysis.
622-646: Correctly implemented FFT segmentation with proper magnitude calculation.The
_fft_segmentedmethod:
- Handles overlapping segments properly
- Applies optional Hanning windowing
- Correctly calculates the magnitude of complex FFT outputs using
Math.hypot(re, im)This implementation addresses one of the key issues mentioned in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/graph_spectrum_calc.js (1)
151-156: 🛠️ Refactor suggestionFFT matrix allocates 2 × more memory than required
Only
magnitudeLengthvalues are written and read, yet each row is created
withfftBufferSize * 2. This doubles RAM usage and cache traffic and makes
the later averaging loop iterate over thousands of untouched zeros.-const matrixFftOutput = new Array(NUM_VS_BINS) - .fill(null) - .map(() => new Float64Array(fftBufferSize * 2)); +const matrixFftOutput = new Array(NUM_VS_BINS) + .fill(null) + .map(() => new Float64Array(magnitudeLength));
🧹 Nitpick comments (3)
src/graph_spectrum_plot.js (2)
372-381:ticksCountis forwarded unchanged – may still be zeroEven with the above guard, callers might still provide
0explicitly.
Consider clamping inside_drawVerticalGridLines()or here:this._drawVerticalGridLines( canvasCtx, LEFT, TOP, WIDTH, HEIGHT, minY, maxY, "dBm/Hz", Math.max(1, ticksCount), // safe forwarding );
1076-1094: UnusedLEFTparameter – remove to simplify the signature
LEFTis never referenced inside_drawVerticalGridLines. Keeping unused
parameters adds noise and invites confusion.-GraphSpectrumPlot._drawVerticalGridLines = function ( - canvasCtx, - LEFT, - TOP, - WIDTH, - HEIGHT, +GraphSpectrumPlot._drawVerticalGridLines = function ( + canvasCtx, + /* LEFT */, /* TOP */, + WIDTH, + HEIGHT, minValue, maxValue, label, ticks = 5, ) {src/graph_spectrum_calc.js (1)
543-574: Heavy window re-allocation inside_psd()
windowis rebuilt for every PSD call. For largepointsPerSegment
(>= 4096) this allocates & initialises tens of kB unnecessarily.Consider caching Hanning windows keyed by
pointsPerSegment
(e.g.this._hanningCache[size]) to reuse the array.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/graph_spectrum_calc.js(10 hunks)src/graph_spectrum_plot.js(14 hunks)
🔇 Additional comments (2)
src/graph_spectrum_plot.js (1)
336-341:⚠️ Potential issueGuard against
ticksCount === 0to avoid division-by-zero in scaling & grid drawingIf the PSD happens to be completely flat (
this._fftData.minimum === this._fftData.maximum) theticksCountbecomes0, which will later be
passed to_drawVerticalGridLines()and used as a divisor inHEIGHT / ticks.
That triggers an Infinity height, completely breaking the rendering.-const ticksCount = (maxY - minY) / dbStep; +let ticksCount = (maxY - minY) / dbStep; +// Ensure at least one tick in degenerate cases +if (ticksCount < 1) { + ticksCount = 1; +}Likely an incorrect or invalid review comment.
src/graph_spectrum_calc.js (1)
348-352: Zero-sample guard for FFT buffer sizingWhen the selected time range is empty,
samplesCountis0;
Math.log2(0)is-Infinity, resulting infftBufferSize = NaNand an
exception downstream.-const fftBufferSize = Math.pow(2, Math.ceil(Math.log2(samplesCount))); +const fftBufferSize = samplesCount > 0 + ? Math.pow(2, Math.ceil(Math.log2(samplesCount))) + : 1; // minimal, prevents NaN
|
The next step will join #827 |
resolved. |
|
This also seems "fast" in the UI (more instantaneous than master). |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/graph_spectrum_plot.js (1)
329-409: New method implementation for Power Spectral Density visualization looks good.The PSD graph rendering is implemented with appropriate axes, scaling, and line drawing. It correctly handles the PSD data format and range.
However, there's one issue on line 359:
🧹 Nitpick comments (2)
index.html (1)
460-464: Verify UI and code alignment for new spectrum options
Ensure that the JavaScript handlers insrc/graph_spectrum_calc.js,src/graph_spectrum_plot.js, andsrc/graph_spectrum.jscorrectly map the expandedspectrumTypeSelectvalues (2–6) to their respective FFT magnitude and PSD computation/rendering logic. Consider adding or updating integration tests to cover the new “Freq. vs RPM”, “Power spectral density”, “PSD vs Throttle”, and “PSD vs RPM” modes.src/graph_spectrum_calc.js (1)
118-120: Add minimum size validation for FFT input.The code should check for minimum size before calculating the FFT to prevent issues with very small or empty sample arrays.
if (pointsPerSegment > flightSamples.samples.length) { - pointsPerSegment = this.getNearPower2Value(flightSamples.samples.length); + // Ensure minimum size for FFT and handle empty arrays + if (flightSamples.samples.length === 0) { + pointsPerSegment = 256; // Default minimum size + } else { + pointsPerSegment = this.getNearPower2Value(flightSamples.samples.length); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
index.html(1 hunks)src/graph_spectrum.js(2 hunks)src/graph_spectrum_calc.js(13 hunks)src/graph_spectrum_plot.js(21 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/graph_spectrum.js (2)
src/graph_spectrum_plot.js (2)
SPECTRUM_TYPE(18-26)SPECTRUM_TYPE(18-26)src/graph_spectrum_calc.js (2)
GraphSpectrumCalc(23-37)GraphSpectrumCalc(23-37)
🔇 Additional comments (28)
src/graph_spectrum.js (3)
115-122: Implementation of new PSD spectrum types looks good.The addition of
PSD_VS_THROTTLEandPSD_VS_RPMcases correctly integrates the new spectrum types defined in theSPECTRUM_TYPEenum. ForPSD_VS_RPM, I notice you're reusing the existingdataLoadFrequencyVsRpmmethod with a boolean parameter instead of creating a new method as you did withdataLoadPowerSpectralDensityVsThrottle.
127-130: LGTM for Power Spectral Density implementation.The
POWER_SPECTRAL_DENSITYcase correctly passes theanalyserZoomYvalue to thedataLoadPSDmethod, which aligns with the PR description of using this value to adjust PSD smoothing.
202-206: Appropriate handling of PSD recalculation on zoom change.Good addition to recalculate PSD data when the Y zoom changes for the
POWER_SPECTRAL_DENSITYtype. This ensures that the visualization correctly reflects the user's adjustment of the smoothing parameter via the vertical slider.src/graph_spectrum_plot.js (12)
3-3: Good addition: Importing NUM_VS_BINS constant from calculation module.This import improves code organization by centralizing the configuration value in the calculation module and reusing it in the plot module.
14-16: LGTM: New constants for PSD y-axis scaling.These constants provide appropriate defaults for dBm value ranges in PSD visualizations.
21-25: LGTM: New spectrum types for PSD visualizations.The enum extension follows the existing pattern and adds clear descriptive types for the new PSD visualization modes.
177-183: LGTM: Added PSD_VS_THROTTLE and PSD_VS_RPM cases to draw method.The implementation correctly reuses the existing _drawFrequencyVsXGraph method with a flag to enable PSD mode.
189-191: LGTM: Added POWER_SPECTRAL_DENSITY case to draw method.The implementation correctly calls the new dedicated method for PSD visualization.
411-416: LGTM: Helper method for retrieving PSD value by frequency.This utility method properly handles bounds checking and provides a clean interface for accessing PSD data in the visualization layer.
418-418: LGTM: Updated frequency vs X graph method with PSD support.The added parameter allows reusing the existing code for both frequency and PSD visualizations.
492-525: LGTM: Enhanced heat map method with PSD scaling.The conditional logic properly handles both frequency and PSD data types with appropriate scaling.
536-546: LGTM: New helper for retrieving values from matrix FFT data.This utility method properly handles bounds checking and provides a clean interface for accessing matrix data.
1525-1534: LGTM: Enhanced mouse position display with PSD values.This enhancement correctly shows PSD values at mouse cursor position, improving user experience.
1573-1583: LGTM: Enhanced mouse position display for PSD vs X graphs.This enhancement correctly shows PSD values for PSD vs throttle/RPM graphs.
1284-1290: LGTM: Enhanced interest frequency labels with PSD values.The conditional logic correctly formats labels for frequency and PSD visualization types.
src/graph_spectrum_calc.js (13)
21-21: LGTM: Exporting NUM_VS_BINS constant.Making this constant available to other modules improves code organization.
91-93: LGTM: Helper function for nearest power of 2 calculation.This utility function correctly calculates the nearest power of 2 value, which is important for optimal FFT performance.
100-100: LGTM: Apply Hanning window only to actual samples.The update correctly ensures the window is only applied to actual data values, not padding zeros.
112-137: LGTM: New PSD calculation implementation.The Power Spectral Density calculation using Welch's method is well-implemented with appropriate segmentation, overlap, and scaling.
147-148: LGTM: Improved FFT buffer sizing and magnitude length calculation.The changes ensure buffer sizes are powers of two for optimal FFT performance and correctly derive the magnitude length.
170-178: LGTM: Corrected magnitude calculation.The implementation now correctly calculates magnitude using both real and imaginary parts of the complex FFT output.
228-298: LGTM: New method for PSD vs throttle/RPM calculation.This implementation correctly adapts the frequency vs X approach to work with PSD calculations.
304-306: LGTM: New method for PSD vs throttle.This helper method correctly calls the underlying implementation with appropriate parameters.
308-314: LGTM: Updated RPM method to support both frequency and PSD.The conditional logic effectively handles both data types with a single interface.
576-611: LGTM: Improved FFT normalization with correct magnitude calculation.The implementation now correctly calculates magnitude using both real and imaginary parts of the complex FFT output.
613-700: LGTM: PSD calculation using Welch's method.The implementation follows the correct mathematical approach for PSD calculation with proper scaling, windowing, and segment averaging.
705-727: LGTM: FFT segmentation for Welch's method.This helper function correctly implements segmented FFT calculation with overlap support.
652-659: LGTM: Early exit for empty segments in PSD calculation.The code correctly handles the edge case of no segments being processed.
|
Ready for review! |
demvlad
changed the title
Was the code seriously doing this in the past? This is crazy and a much needed fix, thats a good find. |
…RPM and PSD by RPM
Co-authored-by: Mark Haslinghuis <[email protected]>
Co-authored-by: Mark Haslinghuis <[email protected]>
Co-authored-by: Mark Haslinghuis <[email protected]>
demvlad
force-pushed
the
fft_input_size_issue
branch
from
9fa94a6 to
fb5a66b
Compare
|
|
Preview URL: https://fb5a66be.betaflight-blackbox.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/graph_spectrum_plot.js (4)
395-400: First line segment starts at (0, 0) instead of the first data point – visible artefact
moveTo(0, 0)draws an unwanted diagonal from the canvas origin to the first
PSD point. Start the path at the correct y-coordinate instead.-canvasCtx.moveTo(0, 0); +const y0 = HEIGHT - (this._fftData.psdOutput[0] - minY) * scaleY; +canvasCtx.moveTo(0, y0);
381-388:ticksCountcan be zero → divide-by-zero downstreamWhen
maxY === minYthe expressionconst ticksCount = (maxY - minY) / dbStep; // → 0propagates to
_drawVerticalGridLines, which divides bytickswhen computing
HEIGHT / ticks.
Guard for this edge case:-const ticksCount = (maxY - minY) / dbStep; +let ticksCount = (maxY - minY) / dbStep; +if (ticksCount < 1) ticksCount = 1; // avoid div/0 and ensure at least one grid line
1162-1172: Robustness of_drawVerticalGridLineswith customticks
ticksis now an argument, but the loop usesHEIGHT / ticksand modulo math
without checking thatticks ≥ 1.
Consider early-returning whenticks <= 0or coercing toMath.max(1, ticks)
to prevent runtime exceptions.
1023-1023: Remove console.warn from production code.Replace with proper error handling or remove if this is an expected condition.
- console.warn("Notch filter fieldName missing"); + // Silently handle missing notch filter fieldName
🧹 Nitpick comments (1)
src/graph_spectrum_calc.js (1)
228-299: Well-implemented PSD analysis with proper binning and scaling.The function correctly implements Welch's method for PSD computation with appropriate background values and matrix sizing. The parallel structure to the frequency analysis provides good consistency.
Consider making the background PSD value a named constant:
+ const BACKGROUND_PSD_VALUE_DB = -200; // Matrix where each row represents a bin of vs values, and the columns are amplitudes at frequencies - const BACKGROUND_PSD_VALUE = -200; + const BACKGROUND_PSD_VALUE = BACKGROUND_PSD_VALUE_DB;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
index.html(2 hunks)src/css/main.css(5 hunks)src/graph_spectrum.js(11 hunks)src/graph_spectrum_calc.js(13 hunks)src/graph_spectrum_plot.js(26 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/css/main.css
- index.html
- src/graph_spectrum.js
🧰 Additional context used
🧠 Learnings (2)
src/graph_spectrum_plot.js (1)
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions `dataLoadPowerSpectralDensityVsThrottle()` and `dataLoadPowerSpectralDensityVsRpm()` instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
src/graph_spectrum_calc.js (1)
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#833
File: src/graph_spectrum.js:0-0
Timestamp: 2025-05-26T16:18:25.863Z
Learning: In src/graph_spectrum_calc.js, the user created dedicated PSD functions `dataLoadPowerSpectralDensityVsThrottle()` and `dataLoadPowerSpectralDensityVsRpm()` instead of using boolean parameters, following a consistent naming pattern for spectrum data loading methods.
🧬 Code Graph Analysis (1)
src/graph_spectrum_plot.js (1)
src/graph_spectrum_calc.js (19)
i(175-175)i(195-195)i(202-202)i(269-269)i(276-276)i(342-342)i(348-348)i(359-359)i(464-464)i(569-569)i(606-606)i(684-684)i(730-730)i(750-750)j(204-204)j(278-278)j(686-686)NUM_VS_BINS(22-22)NUM_VS_BINS(22-22)
🔇 Additional comments (19)
src/graph_spectrum_plot.js (11)
3-3: LGTM! Good constant organization and imports.The import of
NUM_VS_BINSaligns with the PSD functionality, and the new constants provide sensible defaults for dBm range and performance optimization.Also applies to: 15-15, 17-18
20-28: LGTM! Clear spectrum type organization.The addition of PSD spectrum types to the enum is well-organized and maintains logical grouping.
54-56: LGTM! Proper state management with cache invalidation.The PSD parameter setters follow the established pattern of invalidating caches only when values actually change, which is efficient.
Also applies to: 86-111
209-215: LGTM! Consistent switch case handling for new spectrum types.The new case statements properly route PSD spectrum types to their respective drawing methods.
Also applies to: 221-223
262-270: LGTM! Smart performance optimization.The dynamic step calculation effectively limits drawing operations to
MAX_SPECTRUM_LINE_COUNTwhile maintaining visual quality. This should improve rendering performance for large datasets.
447-452: LGTM! Robust helper method with proper bounds checking.The
getPSDbyFreqmethod correctly handles frequency-to-index conversion with appropriate bounds checking and safe array access.
454-454: LGTM! Clean parameter addition with backward compatibility.The
drawPSDparameter with defaultfalsemaintains backward compatibility while enabling PSD-specific behavior.Also applies to: 466-466
501-525: LGTM! Consistent grid line handling for PSD spectrum types.The conditional updates properly include the new PSD spectrum types with appropriate unit labels.
528-557: LGTM! Well-implemented PSD scaling logic.The PSD-specific value clamping and scaling logic is correct, and the scale factor adjustment (1.3 to 1.1) is well-documented with proper reasoning.
573-583: LGTM! Robust matrix access with proper bounds checking.The
getValueFromMatrixFFTmethod correctly handles 2D indexing with appropriate bounds checking and uses the importedNUM_VS_BINSconstant properly.
1298-1299: LGTM! Comprehensive PSD integration throughout the plotting system.The updates to background gradient opacity, interest frequency labeling, and mouse position handling properly integrate PSD functionality with appropriate value displays and units.
Also applies to: 1320-1327, 1568-1577, 1590-1592, 1616-1626
src/graph_spectrum_calc.js (8)
20-22: Well-structured constants and exports.The new constants are appropriately named and the export of
NUM_VS_BINSprovides good encapsulation for shared values across modules.
94-96: Function correctly implements power-of-2 sizing for FFT optimization.The implementation properly returns the next power of 2, which is used correctly throughout the codebase for FFT buffer allocation with zero-padding.
115-141: Excellent implementation of PSD analysis using Welch's method.The function properly handles segment sizing based on zoom level, implements appropriate overlap (50% when possible), and includes necessary bounds checking. The return structure provides comprehensive PSD data for visualization.
172-180: Critical fix: Correct FFT magnitude calculation implemented.The use of
Math.hypot(re, im)to compute the Euclidean norm of complex FFT output is mathematically correct and addresses the previous issue where only real parts were used.
191-191: Elegant bin index calculation eliminates bounds checking.Using
(NUM_VS_BINS - 1)as the multiplier ensures the result naturally falls within [0, NUM_VS_BINS-1] range, making the code cleaner and more robust.
606-614: Correct magnitude calculation and noise detection implemented.The use of
Math.hypot(re, im)for magnitude calculation is mathematically correct, and the noise detection logic properly identifies dominant frequencies above the low-end threshold.
630-719: Mathematically rigorous implementation of Welch's method for PSD computation.This function correctly implements all aspects of Welch's method including:
- Proper scaling for windowed/non-windowed cases
- Correct one-sided spectrum conversion (doubling except DC and Nyquist)
- Appropriate dB conversion with minimum value clamping
- Comprehensive noise frequency detection
The implementation follows signal processing standards and provides robust PSD analysis.
721-759: Well-implemented segmented FFT processing for Welch's method.The function correctly handles overlapping segments and includes a smart optimization for single-segment cases by extending to power-of-2 sizes. The magnitude calculation using
Math.hypotis consistent and correct.
This PR shows my questions and improving suggestions to compute spectrums data and the charts plot.
The spectrum improvement work started in the next draft PRs:
#819, #820, #821, #826, #827
There are my opinion about current spectrum in master branch after some research
I am not expert in FFT, i study it step by step til.
I would appreciate any help. @KarateBrot , @Quick-Flash , @ledvinap @ctzsnooze @mituritsyn
The previous improving PRs used actual samples count as fft input (#819). Its worked, but had slow hang fft computing sometime. This issue was reason of this PR.
This PR uses dynamical power at 2 fft input size. The samples input size extended to the next power at 2 value by using zero padding. Probably, it needs to limit minimal size value to prevent low fft resolution. It will give balance between speed and quality. Will look.
After improvement of fft computing, this PR joins the previous work:
Resolved issue of magnitude computing at the spectrum charts (Resolved issue of magnitude computing at the spectrum charts #821 )
The spectrum magnitude is computed by complex Re and Im value as Math.hypot(Re, Im)
The log file:
Uploading btfl_all.txt…
The log files what had very slow spectrum drawing:
Uploading slow_log.txt…
log.txt
Its work fast now.
Power spectral density curves (Added power spectral density curves at the spectrum chart #820). I tryed to implement Welch PSD computing method follow python SciPy library and Python Spectral Analysis tutorial.
The PSD value shows in the standard dBm/Hz units. The charts show maximal noise point and have mouse cursor to meassure current mouse position..
The vertical slider changes curves smoothing (changes samples per segment value in the Welch method).
Power spectral density vs throttle and RPM heat map charts Added power spectral density charts #827
The charts have mouse cursor to meassure current mouse position.. The heat map shows PSD values in the standard dBm/Hz units as red color map by using scaling: -40dBm/Hz is black and +10dBm/Hz is white colors. The logoryphmic PSD can show the all values range at the heat map chart.
Summary by CodeRabbit