The spectrum charts improvements. #833
Conversation
|
""" WalkthroughThe changes standardize FFT buffer sizes to powers of two throughout spectrum calculation functions, update array allocation, slicing, and bin indexing accordingly, and add a new method for Power Spectral Density (PSD) computation using Welch's method. Several function signatures are updated to reflect these changes, and comments highlight needed corrections in magnitude calculation. Additionally, a new PSD graph type with rendering, labeling, and interaction support is implemented. The HTML spectrum type selector is updated to include new PSD options, and the main analysis logic incorporates these new spectrum types with dynamic updates on zoom changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GraphSpectrumCalc
participant FFT
User->>GraphSpectrumCalc: dataLoadFrequency()
GraphSpectrumCalc->>FFT: Prepare buffer (power-of-two size), zero-pad if needed
FFT-->>GraphSpectrumCalc: FFT output
GraphSpectrumCalc->>GraphSpectrumCalc: _normalizeFft(fftOutput)
GraphSpectrumCalc-->>User: Normalized spectrum
User->>GraphSpectrumCalc: dataLoadPSD(analyserZoomY)
GraphSpectrumCalc->>FFT: Prepare segment (largest power-of-two ≤ sample size)
FFT-->>GraphSpectrumCalc: FFT output for each segment
GraphSpectrumCalc->>GraphSpectrumCalc: Aggregate PSD results with overlap and windowing
GraphSpectrumCalc-->>User: PSD data
User->>GraphSpectrumPlot: Select POWER_SPECTRAL_DENSITY spectrum type
GraphSpectrumPlot->>GraphSpectrumPlot: _drawPowerSpectralDensityGraph(canvasCtx)
GraphSpectrumPlot-->>User: Render PSD graph with labels and max noise marker
Poem
✨ 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 (
|
![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 |
✅ Actions performedFull review triggered. |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/graph_spectrum_plot.js (1)
329-364:⚠️ Potential issueFirst line segment starts at (0, 0) instead of the first data point – visible artifact.
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);
🧹 Nitpick comments (2)
index.html (1)
460-464: Align dropdown values with JSspectrumTypeenum and improve grouping
Ensure that the numeric values (2–6) for the new options exactly match the IDs used in your JavaScriptspectrumTypeenum and switch/case logic—any discrepancy will break chart selection. For clearer UX, consider wrapping the PSD entries in an<optgroup label="Power Spectral Density">…</optgroup>and separating the “Error vs Setpoint” into its own group.src/graph_spectrum_calc.js (1)
226-294: Well-structured implementation of PSD vs X calculation.The implementation parallels the structure of
_dataLoadFrequencyVsXbut with appropriate adaptations for PSD calculation. Key strengths include:
- Reuse of common patterns for bin index calculation
- Proper handling of background values
- Correct accumulation and averaging of PSD values per bin
One minor issue is the use of the magic number
-200as a background value (line 238), which could be defined as a constant for better maintainability.Consider defining a named constant for the background PSD value:
-const backgroundValue = -200; +const BACKGROUND_PSD_VALUE = -200; // dBm/Hz, effectively -infinity for PSD
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 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 comments (37)
src/graph_spectrum.js (3)
115-122: Good addition of new spectrum types for PSD analysis.The implementation correctly adds support for the new Power Spectral Density (PSD) spectrum types by integrating with the appropriate data loading methods from GraphSpectrumCalc.
127-130: LGTM - Consistent handling of the new POWER_SPECTRAL_DENSITY type.The implementation correctly integrates the new PSD spectrum type into the existing spectrum selection logic.
202-206: Essential update for dynamic PSD recalculation on zoom changes.This is a crucial addition that ensures the PSD is recalculated when the vertical zoom level changes. This is necessary because the segment sizes for Welch's method of PSD calculation are affected by the zoom level, making this dynamic update essential for accurate visualization.
src/graph_spectrum_plot.js (17)
14-16: Good addition of constants for dBm range.Defining MIN_DBM_VALUE and MAX_DBM_VALUE as constants improves code maintainability and makes the dBm range easily adjustable throughout the codebase.
21-25: LGTM - Clean extension of the SPECTRUM_TYPE enum.The new spectrum types are logically organized and maintain backward compatibility with the existing enum values.
177-183: Good integration of PSD spectrum types with drawing functions.The implementation efficiently handles the new PSD-related spectrum types by reusing the existing frequency vs X graph drawing function with a PSD flag, which reduces code duplication.
189-191: LGTM - Clean implementation of PSD graph drawing.This new case correctly handles the POWER_SPECTRAL_DENSITY spectrum type with a dedicated drawing function.
209-210: Doubled the bar density with halved step size.The change from a step size of 10 to 5 in the FFT visualization doubles the bar density, providing a more detailed spectrum display without overly crowding the visualization.
233-237: LGTM - Updated loop step size to match bar density.This change to iterate by 5 instead of 10 properly matches the updated bar width calculation.
411-416: Good helper method to get PSD value by frequency.This utility method enables retrieving PSD values for a given frequency, useful for interaction features and accurate value readouts when hovering over the graph.
418-421: LGTM - Flexible implementation with drawPSD parameter.The updated method signature with an optional
drawPSDparameter allows reusing the existing frequency vs X graph drawing function for both standard FFT and PSD visualization, reducing code duplication.
465-478: Clean update to include PSD types in vertical grid lines logic.The conditional statements are properly extended to handle the new PSD-related spectrum types when drawing vertical grid lines.
492-520: Good implementation of dual-purpose heat map scaling.The heat map drawing function now properly handles both standard FFT and PSD data with appropriate value clamping and scaling. The comment on line 494-495 correctly explains why the scale factor was reduced from 1.3 to 1.1 to account for the magnitude calculation change.
536-546: Good helper for retrieving matrix FFT values.This utility method provides a clean API for accessing matrix FFT values by frequency and vertical scale argument with appropriate bounds checking.
1117-1126: LGTM - Flexible vertical grid lines with configurable ticks.The addition of an optional
ticksparameter to the_drawVerticalGridLinesmethod makes the grid line drawing more flexible and configurable.
1261-1262: Improved contrast with increased opacity for fullscreen mode.Increasing the opacity to 1.0/0.9 from 0.9/0.7 provides better contrast and readability in fullscreen mode.
1284-1290: Enhanced interest frequency labels with PSD values.For the POWER_SPECTRAL_DENSITY type, the interest frequency label now includes both frequency and PSD values with appropriate units, significantly improving the information available to users.
1525-1534: Good implementation of PSD value display at cursor position.This enhancement shows the PSD value at the mouse position, providing essential feedback when analyzing the spectrum.
1573-1583: LGTM - Clean PSD value readout for VS graphs.The implementation properly displays PSD values for PSD_VS_THROTTLE and PSD_VS_RPM graphs when hovering with the mouse, improving the user experience.
1637-1639: Properly updated margin calculation for new spectrum types.The margin left calculation now includes the new PSD spectrum types in the group that uses a nonzero left margin, ensuring consistent layout across all related spectrum types.
src/graph_spectrum_calc.js (17)
21-21: Good decision to export NUM_VS_BINS constant.Exporting this constant makes it available to other modules, facilitating consistent bin counts across the codebase and improving maintainability.
81-81: Fixed a bug in setOutTime method.The code correctly updates the reference to
this._analyserTimeRange.inwhen calculating the maximum allowed out time. This fixes a potential bug where the wrong value might be used.
91-93: Good utility for finding next power-of-two value.This helper function standardizes the calculation of power-of-two buffer sizes throughout the code, making the FFT calculations more efficient and consistent.
100-100: Improved Hanning window application with explicit length parameter.The code now explicitly passes the sample count to
_hanningWindow, ensuring that only the actual data points (not padding) receive the window function.
107-107: LGTM - Simplified normalization with no explicit size parameters.The code now passes just the output to
_normalizeFftwithout explicit size parameters, as they can be derived from the array itself. This simplifies the API and reduces the chance of errors.
112-137: Excellent implementation of Power Spectral Density calculation.The
dataLoadPSDfunction implements Welch's method for Power Spectral Density estimation using overlapping segments of power-of-two size. The segment size adjustment based on the vertical zoom level (analyserZoomY) allows users to balance between frequency resolution and variance reduction.Two points worth noting:
- The segment size doubling with zoom levels provides a logical progression
- The 50% overlap is a good standard choice for PSD estimation
146-148: LGTM - Optimized FFT buffer sizes and magnitude length calculation.The changes use power-of-two buffer sizes for FFT calculation, which significantly improves FFT performance. The magnitude length is correctly set to half the buffer size to account for the symmetrical nature of the FFT output.
156-161: Good buffer allocation with proper zero-padding.The implementation correctly creates buffers of power-of-two size and uses
set()to copy actual sample data, effectively zero-padding the input data for optimal FFT performance.
169-178: Crucial fix to FFT magnitude calculation.This change is essential for accurate spectral analysis. Instead of incorrectly using only the real part of the FFT output, the code now properly computes the magnitude as the Euclidean norm (hypotenuse) of the real and imaginary parts using
Math.hypot(re, im).
189-189: Improved bin index calculation prevents array bounds issues.Using
Math.round((NUM_VS_BINS - 1) * ...)instead ofMath.round(NUM_VS_BINS * ...)is an elegant solution that naturally constrains the result to be within the valid array index range [0, NUM_VS_BINS-1], eliminating the need for a separate bounds check.
300-302: LGTM - Clean implementation of throttle-based PSD.This function provides a simple interface to the more general
_dataLoadPowerSpectralDensityVsXmethod with appropriate parameters for throttle.
304-310: Elegant dual-purpose implementation with flag parameter.The updated method efficiently handles both standard FFT and PSD cases with a single boolean parameter, reducing code duplication and simplifying the API.
393-418: Improved flight samples retrieval with optional scaling.The updated method now:
- Accepts an optional
scaledparameter to allow bypassing the curve lookup scaling- Uses the proper power-of-two buffer size
- Correctly slices the samples array to match the buffer size
These changes improve both flexibility and efficiency.
430-506: LGTM - Consistent improvements to frequency vs X samples retrieval.The changes parallel those in
_getFlightSamplesFreq, adding optional scaling and proper array slicing. The consistent pattern across these related methods improves maintainability.
573-596: Critical fix to FFT magnitude calculation in normalization.This change correctly computes the magnitude from complex FFT output pairs using
Math.hypot(re, im)rather than just the real components. The enhanced detection of maximum noise frequency also improves marker placement for visualization.
609-696: Excellent implementation of Welch's method for PSD estimation.This comprehensive implementation:
- Computes FFT for multiple overlapping segments
- Correctly applies window function scaling
- Converts power to dB scale (10*log10)
- Handles edge cases and provides accurate min/max tracking
- Determines the maximum noise frequency
The early exit at line 648 for zero segments is a good defensive programming practice.
698-723: Well-structured implementation of segmented FFT calculation.This helper function cleanly handles the process of dividing input data into potentially overlapping segments, optionally applying a window function, and computing the FFT magnitude for each segment. The use of array slicing and proper magnitude calculation from complex pairs ensures accuracy.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/graph_spectrum_calc.js (2)
112-137: Well-implemented PSD calculation with Welch's method.The new
dataLoadPSDmethod correctly implements Power Spectral Density calculation using Welch's method with adjustable smoothing (via analyserZoomY). The variable segment size approach based on zoom level is a good design that balances performance vs. resolution.However, consider adding a minimum size check for very small samples:
// Use power 2 fft size what is not bigger flightSamples.samples.length if (pointsPerSegment > flightSamples.samples.length) { pointsPerSegment = this.getNearPower2Value(flightSamples.samples.length); + // Ensure minimum size for adequate frequency resolution + pointsPerSegment = Math.max(pointsPerSegment, 256); }
146-150: Optimize matrix memory allocation.While the FFT calculations now use the correct magnitude calculation, the
matrixFftOutputarray is still sized tofftBufferSize * 2, which is larger than needed since you're only storing magnitudes of lengthmagnitudeLengthin each bin.Consider reducing the memory allocation:
- 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));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/graph_spectrum_calc.js(13 hunks)
🔇 Additional comments (9)
src/graph_spectrum_calc.js (9)
21-21: Good practice exporting the constant.Exporting NUM_VS_BINS is a good practice since this constant is likely needed by other modules that interact with the spectrum calculation code. This promotes consistency and reduces the risk of magic numbers throughout the codebase.
91-93: Great FFT optimization technique.The
getNearPower2Valuemethod efficiently calculates the next power of 2 for a given input size, which is a standard optimization for FFT calculations. Using power-of-2 sizes significantly improves FFT performance due to the way the algorithm divides and conquers.
169-178: Excellent FFT magnitude computation improvement.This code correctly calculates the magnitude of complex FFT outputs using
Math.hypot(re, im)instead of just taking the absolute value of the real part. This addresses a critical issue identified in previous reviews and ensures mathematically accurate spectrum magnitude calculations.
189-189: Smart bin index calculation improvement.This is an excellent improvement to the bin index calculation. Using
Math.round((NUM_VS_BINS - 1) * ...)instead ofMath.round(NUM_VS_BINS * ...)is more elegant because:
- It ensures the maximum possible bin index is exactly
NUM_VS_BINS - 1whenavgVsValueequalsflightSamples.maxValue- It eliminates the need for a bounds check after calculation
- The code becomes cleaner and more direct
238-242: Good approach for background values.Using a specific background value (BACKGROUND_PSD_VALUE) for initializing the PSD output matrix is a good approach for visualizing missing data points in the heatmap. This clear distinguishing value (-200 dBm/Hz) helps identify areas with no data versus those with measured low power.
259-259: Consistent bin indexing across methods.Good job maintaining consistency by using the same improved bin index calculation formula in both frequency and PSD processing methods. Consistent binning behavior is crucial for comparable visualizations between different spectrum types.
583-593: Fixed FFT magnitude calculation in normalization.This correctly fixes the magnitude calculation in the
_normalizeFftmethod, addressing the issue mentioned in previous reviews. UsingMath.hypot(re, im)to calculate the proper complex magnitude ensures mathematically accurate spectrum representation.
609-696: Well-implemented PSD calculation with industry standard approaches.The
_psdmethod provides a thorough implementation of Welch's method for PSD calculation, which is the standard approach for reducing noise in spectral estimates. The implementation includes:
- Proper scaling based on window function and scaling type (density vs. spectrum)
- Segment averaging to reduce variance
- Appropriate conversion to dB scale using 10*log10
- Noise peak detection logic
- Minimum value thresholding to avoid log(0) issues
The overall implementation follows scientific best practices for spectral analysis.
701-723: Clean implementation of segmented FFT with overlap.The
_fft_segmentedmethod efficiently breaks input data into segments with overlap, applies window functions as needed, and computes magnitude spectra for each segment. This helper function is well organized and properly supports the Welch method implementation above.
|
The small improvements: |
✅ Actions performedReview triggered.
|
|
moving vertical scale lower than default position has the unexpected effect of making us feel like we've increased it |
|
master: |
|
vertical scale doesn't affect PSD vs X plots - is it intended? |
IMO we no need to scale dBm units PSD values. Its show the wide -40...+10dBm values range. The max/min=10e5 in linear non log scale. |
|
Maybe use short "PSD" caption instead of full Power.... |
The lower number per segment values has low frequency resolution at the chart and rude curves look. Therefore i've limited it values by down slider moving |
Exactly, we need to see noise in the filtered data. |
Ok. Will try to do shifts by 10dBm step into "dark" area, will look as -50...0, -60...-10, -70...-20... dBm range by slider up movement, and some steps into light area - -30...20, -20...30 dBm by slider down. |
Resolved. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/graph_spectrum_plot.js (1)
329-409: 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);
🧹 Nitpick comments (2)
src/graph_spectrum_plot.js (2)
492-529: Simplify the dBm range shift calculation logic.The current implementation for calculating the dBm range shift is complex and difficult to follow. Consider extracting this logic into a separate helper method with clearer intention.
- //Compute the dbm range shift from zoomY as[-30, -20, -10, 0, +10, +20] - let dBmRangeShift = Math.floor(1 / this._zoomY) - 1; // -1 ... 9 - if (dBmRangeShift > 0) { - dBmRangeShift = -10 * Math.round(dBmRangeShift / 3); //-10, -20, -30 - } else if (dBmRangeShift < 0) { - dBmRangeShift = -10 * Math.round(dBmRangeShift * 2); //+10, +20 - } - const dBmValueMin = MIN_DBM_VALUE + dBmRangeShift, - dBmValueMax = MAX_DBM_VALUE + dBmRangeShift; + const dBmRangeShift = this._calculateDBmRangeShift(this._zoomY); + const dBmValueMin = MIN_DBM_VALUE + dBmRangeShift, + dBmValueMax = MAX_DBM_VALUE + dBmRangeShift; // Then add this helper method elsewhere in the class: +GraphSpectrumPlot._calculateDBmRangeShift = function(zoomY) { + // Calculate dBm range shift from zoomY as[-30, -20, -10, 0, +10, +20] + let shift = Math.floor(1 / zoomY) - 1; // -1 ... 9 + + if (shift > 0) { + // For values > 0, shift by -10, -20, or -30 dBm + return -10 * Math.round(shift / 3); + } else if (shift < 0) { + // For values < 0, shift by +10 or +20 dBm + return -10 * Math.round(shift * 2); + } + + // No shift (0) for neutral zoom level + return 0; +}
545-555: Add input validation to the matrix FFT value retrieval.This method should validate input parameters to prevent potential issues with invalid frequency or vsArgument values.
GraphSpectrumPlot.getValueFromMatrixFFT = function(frequency, vsArgument) { + if (typeof frequency !== 'number' || isNaN(frequency) || + typeof vsArgument !== 'number' || isNaN(vsArgument)) { + console.warn("Invalid parameters for getValueFromMatrixFFT:", frequency, vsArgument); + return 0; + } + const matrixFFT = this._fftData; + if (!matrixFFT || !matrixFFT.fftOutput || !matrixFFT.vsRange) { + return 0; + } + let vsArgumentIndex = Math.round(NUM_VS_BINS * (vsArgument - matrixFFT.vsRange.min) / (matrixFFT.vsRange.max - matrixFFT.vsRange.min)); if (vsArgumentIndex >= NUM_VS_BINS) { vsArgumentIndex = NUM_VS_BINS - 1; } + vsArgumentIndex = Math.max(0, vsArgumentIndex); + let freqIndex = Math.round(2 * frequency / matrixFFT.blackBoxRate * (matrixFFT.fftLength - 1)); freqIndex = Math.max(freqIndex, 0); freqIndex = Math.min(freqIndex, matrixFFT.fftLength - 1); return matrixFFT.fftOutput[vsArgumentIndex][freqIndex]; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/graph_spectrum_plot.js(24 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/graph_spectrum_plot.js (1)
src/graph_spectrum_calc.js (19)
i(173-173)i(193-193)i(200-200)i(263-263)i(270-270)i(331-331)i(337-337)i(348-348)i(446-446)i(551-551)i(588-588)i(664-664)i(708-708)i(717-717)j(202-202)j(272-272)j(666-666)NUM_VS_BINS(21-21)NUM_VS_BINS(21-21)
🔇 Additional comments (10)
src/graph_spectrum_plot.js (10)
3-3: LGTM: Imported the necessary constant.The
NUM_VS_BINSconstant imported fromgraph_spectrum_calc.jsis appropriately used in thegetValueFromMatrixFFTmethod for frequency vs. value calculations.
14-16: LGTM: Constants for dBm range of Power Spectral Density.These constants define the default range for Power Spectral Density (dBm/Hz) visualization, with appropriate values for typical signal strength measurements.
21-25: LGTM: Extended spectrum type enum with new PSD-related options.Good enhancement of the spectrum type enumeration to include new Power Spectral Density (PSD) related visualization options.
177-191: LGTM: Added support for new PSD visualization types.The switch statement has been appropriately extended to handle the new PSD spectrum types, reusing existing draw methods with the PSD flag where possible.
411-416: LGTM: Good helper method for retrieving PSD value by frequency.This is a well-implemented utility function that correctly handles edge cases by constraining the frequency index within array bounds.
418-430: LGTM: Enhanced function to support both FFT and PSD visualization.Good approach to reuse the existing frequency vs. X graph drawing functionality for PSD, using a boolean parameter to control the behavior.
1292-1299: LGTM: Added informative frequency labels with PSD values.Good enhancement to show PSD values alongside frequency when in PSD mode, providing more useful information to the user.
1515-1549: LGTM: Enhanced mouse position display for PSD visualizations.The code properly handles displaying PSD values when the mouse hovers over the graph, making the PSD visualization more interactive and informative.
1588-1598: LGTM: Added PSD value display for frequency vs. throttle/RPM visualizations.Good addition to show PSD values when hovering over the heatmap, providing useful measurement capabilities.
1652-1654: LGTM: Updated margin handling for new spectrum types.The code correctly adds the new PSD spectrum types to the margin calculation logic, ensuring consistent layout across visualization types.
demvlad
force-pushed
the
fft_input_size_issue
branch
from
1b9eccd to
257f5cc
Compare
|
|
Preview URL: https://257f5cc8.betaflight-blackbox.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/graph_spectrum_plot.js (1)
329-409:⚠️ Potential issueFix the PSD graph path starting point.
The moveTo(0, 0) call in line 359 creates an unwanted diagonal line from the origin to the first PSD point. This creates a visible artifact in the rendered graph.
Apply this fix:
-canvasCtx.moveTo(0, 0); +const y0 = HEIGHT - (this._fftData.psdOutput[0] - minY) * scaleY; +canvasCtx.moveTo(0, y0);
🧹 Nitpick comments (1)
src/graph_spectrum_plot.js (1)
465-479: Consider refactoring repeated spectrum type checks.The pattern of checking for specific spectrum types (FREQ_VS_THROTTLE, PSD_VS_THROTTLE, FREQ_VS_RPM, PSD_VS_RPM) is repeated in multiple places in the code. Consider creating helper methods to simplify these checks.
+// Add these helper methods to GraphSpectrumPlot +GraphSpectrumPlot._isThrottleBasedSpectrum = function() { + return this._spectrumType === SPECTRUM_TYPE.FREQ_VS_THROTTLE || + this._spectrumType === SPECTRUM_TYPE.PSD_VS_THROTTLE; +}; + +GraphSpectrumPlot._isRpmBasedSpectrum = function() { + return this._spectrumType === SPECTRUM_TYPE.FREQ_VS_RPM || + this._spectrumType === SPECTRUM_TYPE.PSD_VS_RPM; +}; // Then use them in conditions like: -if (this._spectrumType === SPECTRUM_TYPE.FREQ_VS_THROTTLE || - this._spectrumType === SPECTRUM_TYPE.PSD_VS_THROTTLE) { +if (this._isThrottleBasedSpectrum()) { // ... -} else if (this._spectrumType === SPECTRUM_TYPE.FREQ_VS_RPM || - this._spectrumType === SPECTRUM_TYPE.PSD_VS_RPM) { +} else if (this._isRpmBasedSpectrum()) { // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/graph_spectrum_plot.js(24 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/graph_spectrum_plot.js (1)
src/graph_spectrum_calc.js (19)
i(173-173)i(193-193)i(200-200)i(263-263)i(270-270)i(331-331)i(337-337)i(348-348)i(446-446)i(551-551)i(588-588)i(664-664)i(708-708)i(717-717)j(202-202)j(272-272)j(666-666)NUM_VS_BINS(21-21)NUM_VS_BINS(21-21)
🔇 Additional comments (14)
src/graph_spectrum_plot.js (14)
3-3: Good addition of the NUM_VS_BINS import.Adding this import ensures consistency with the FFT data structure defined in graph_spectrum_calc.js and is needed for accurate indexing of vertical spectrum bins.
14-16: Good addition of constants for zoom and dBm value ranges.These constants provide clear boundaries for the power spectral density measurements and improve code readability by replacing magic numbers with named constants.
21-25: Good extension of the SPECTRUM_TYPE enum with new PSD display options.The new spectrum types properly extend the visualization capabilities to include Power Spectral Density (PSD) and its variations with throttle and RPM.
177-191: Good implementation of new spectrum type handling.The switch statement has been correctly extended to handle the new PSD spectrum types, reusing the existing vs-X graph rendering function with a PSD flag for throttle and RPM variants, and implementing a dedicated method for the base PSD visualization.
209-209: Good adjustment to bar width calculation for better visualization.The updated calculation for bar width ensures proper spacing and visualization of frequency data in the FFT display.
411-416: Good implementation of PSD frequency lookup.This helper method efficiently retrieves the PSD value for a specific frequency, handling bounds checking and edge cases appropriately.
418-418: Good enhancement with optional PSD parameter.Adding the optional
drawPSDparameter to the_drawFrequencyVsXGraphmethod allows reusing the same function for both regular and PSD spectrum types, reducing code duplication.
492-530: Good implementation of PSD heatmap scaling.The heatmap generation has been enhanced to support PSD visualization with appropriate scaling between dBm values. The comment on line 494-495 correctly explains the magnitude adjustment for the heatmap scale.
The dBm range shifting based on zoom levels (lines 507-515) is a clever approach to allow users to focus on different parts of the spectrum by adjusting the vertical zoom.
545-555: Good implementation of matrix FFT value lookup.This helper method correctly handles the mapping between frequency/vertical coordinates and their corresponding indices in the FFT data matrix, with appropriate bounds checking.
1126-1136: Good enhancement of vertical grid lines with configurable ticks.The addition of the optional
ticksparameter makes the vertical grid lines more flexible and adaptable to different visualization requirements, especially for the PSD display which may need different tick counts.
1292-1300: Good enhancement of interest frequency labeling for PSD.The code now displays both the frequency and corresponding PSD value for interest markers when in PSD mode, providing more valuable information to the user.
1515-1549: Good implementation of PSD value display in mouse position.This enhancement shows the PSD value at the mouse cursor position, greatly improving usability for analyzing specific frequency components in the spectrum.
1588-1599: Good implementation of matrix PSD value display for VS types.This addition shows the PSD value at the intersection of frequency and throttle/RPM when hovering over PSD heat maps, making the visualization much more informative.
1647-1671: Good margin handling for new spectrum types.The _getActualMarginLeft method has been properly updated to handle the new PSD spectrum types, ensuring consistent layout across all visualization modes.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1665-1665: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
|
The vertical slider works at the PSD heatmap charts. |
|
Functionally, it looks fine. |
Thank's! |
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
Summary by CodeRabbit