Added power spectral density curves at the spectrum chart #820
Conversation
demvlad
force-pushed
the
psd_spectrum
branch
2 times, most recently
from
0cce938 to
ee88e41
Compare
|
the smoothing seems to be rather different in the BBE vs the PTB graphs. |
Yes. I am using too small point per segment and big overlap values probably. It needs to find some optimal settings. |
|
@mituritsyn |
src/graph_spectrum_calc.js
Outdated
| if (dataCount % 2) { | ||
| p *= 2; | ||
| } else if (i != dataCount - 1) { | ||
| p *= 2; | ||
| } |
There was a problem hiding this comment.
should we perform same action for both if cases?
There was a problem hiding this comment.
The first condition is removed. The fft output has even size only for our fft transform procedure
Co-authored-by: MikeNomatter <[email protected]>
|
Caution Review failedThe pull request is closed. """ WalkthroughA new option labeled "Power spectral density" was added to the spectrum type dropdown in the user interface. The data loading logic was extended to support this new spectrum type by invoking a new method that computes the Power Spectral Density (PSD) using Welch's method. Helper functions were added for segmenting FFT calculations and computing PSD with appropriate scaling. The plotting module was updated to recognize the PSD spectrum type and render the corresponding graph with frequency and power density axes, grid lines, labels, and a marker for maximum noise frequency. Minor formatting improvements were made in the plotting code. No existing features were removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant GraphSpectrum
participant GraphSpectrumCalc
participant GraphSpectrumPlot
User->>UI: Selects "Power spectral density" in dropdown
UI->>GraphSpectrum: Calls dataLoad with POWER_SPECTRAL_DENSITY
GraphSpectrum->>GraphSpectrumCalc: Calls dataLoadPSD(zoom)
GraphSpectrumCalc->>GraphSpectrumCalc: Computes PSD using Welch's method
GraphSpectrumCalc-->>GraphSpectrum: Returns PSD data
GraphSpectrum->>GraphSpectrumPlot: Calls _drawPowerSpectralDensityGraph()
GraphSpectrumPlot->>GraphSpectrumPlot: Renders PSD graph on canvas
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ 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: 5
♻️ Duplicate comments (1)
index.html (1)
462-463: Consider renaming for UX consistency.
Other dropdown entries use an “X vs Y” pattern (e.g. “Freq. vs Throttle”); you might prefer a label like"Frequency vs noise level"or"Noise level vs Frequency"to align with the existing naming convention.
🧹 Nitpick comments (1)
src/graph_spectrum_calc.js (1)
528-553: Window-scaling formula deviates from reference implementationFor
scaling === 'density', SciPy uses
scale = 1 / (fs * Σw²)(OK), not1 / n_per_segwhen no window is applied – it should still be divided byfs.Likewise, in
'spectrum'modescale = 1 / (Σw)², independent offs. Your fallback1 / n_per_seg ** 2is close, but would benefit from mirroring the reference formulas for consistency:- } else if (!userSettings.analyserHanning) { - scale = 1 / n_per_seg; + } else if (!userSettings.analyserHanning) { + scale = 1 / (fs * n_per_seg); } ... - } else { - scale = 1 / n_per_seg ** 2; + } else { + scale = 1 / (n_per_seg ** 2); }Not critical for visualisation, but necessary for unit correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
index.html(1 hunks)src/graph_spectrum.js(1 hunks)src/graph_spectrum_calc.js(4 hunks)src/graph_spectrum_plot.js(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/graph_spectrum.js (2)
src/graph_spectrum_plot.js (2)
SPECTRUM_TYPE(15-21)SPECTRUM_TYPE(15-21)src/graph_spectrum_calc.js (6)
fftData(104-104)fftData(203-211)fftData(222-222)fftData(504-511)GraphSpectrumCalc(24-38)GraphSpectrumCalc(24-38)
🔇 Additional comments (5)
index.html (1)
462-463: Approve adding PSD to spectrum type dropdown.
The new<option value="4">Power spectral density</option>correctly surfaces the PSD feature in the UI, matching the correspondingSPECTRUM_TYPE.PSD_VS_FREQUENCYhandling in the code.src/graph_spectrum.js (1)
119-122: Implementation looks good! Adds proper PSD support to switch case.This change properly adds the new
PSD_VS_FREQUENCYcase to load Power Spectral Density data, following the same pattern as other spectrum types.src/graph_spectrum_plot.js (3)
20-20: LGTM! Added new spectrum type enum value.Adding the new enum value for Power Spectral Density is correct and follows the existing pattern.
176-178: LGTM! Added case for new spectrum type.This change correctly adds the new case to draw the PSD graph, matching the pattern used for other spectrum types.
302-303: Minor style improvements.Small formatting improvements for consistency with trailing commas.
Also applies to: 312-314
| if (scaled) { | ||
| samples[samplesCount] = (this._dataBuffer.curve.lookupRaw(frame[this._dataBuffer.fieldIndex])); | ||
| } else { | ||
| samples[samplesCount] = frame[this._dataBuffer.fieldIndex]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid lookupRaw when no curve is present
When scaled === false, the code falls back to raw frame values (good).
However, when scaled === true it unconditionally calls this._dataBuffer.curve.lookupRaw(...). If curve is null (e.g., for already-scaled fields), this will throw.
-if (scaled) {
- samples[samplesCount] = (this._dataBuffer.curve.lookupRaw(frame[this._dataBuffer.fieldIndex]));
+if (scaled && this._dataBuffer.curve) {
+ samples[samplesCount] = this._dataBuffer.curve.lookupRaw(
+ frame[this._dataBuffer.fieldIndex]
+ );
} else {
samples[samplesCount] = frame[this._dataBuffer.fieldIndex];
}Preventing the null-dereference keeps existing FFT paths stable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (scaled) { | |
| samples[samplesCount] = (this._dataBuffer.curve.lookupRaw(frame[this._dataBuffer.fieldIndex])); | |
| } else { | |
| samples[samplesCount] = frame[this._dataBuffer.fieldIndex]; | |
| } | |
| if (scaled && this._dataBuffer.curve) { | |
| samples[samplesCount] = this._dataBuffer.curve.lookupRaw( | |
| frame[this._dataBuffer.fieldIndex] | |
| ); | |
| } else { | |
| samples[samplesCount] = frame[this._dataBuffer.fieldIndex]; | |
| } |
src/graph_spectrum_calc.js
Outdated
| GraphSpectrumCalc.dataLoadFrequencyPSD = function() { | ||
| const points_per_segment = 512, | ||
| overlap_count = 256; | ||
| const flightSamples = this._getFlightSamplesFreq(false); | ||
| const psd = this._psd(flightSamples.samples, this._blackBoxRate, points_per_segment, overlap_count); | ||
| let min = 1e6, | ||
| max = -1e6; | ||
| for (const value of psd) { | ||
| min = Math.min(value, min); | ||
| max = Math.max(value, max); | ||
| } | ||
|
|
||
| const psdData = { | ||
| fieldIndex : this._dataBuffer.fieldIndex, | ||
| fieldName : this._dataBuffer.fieldName, | ||
| psdLength : psd.length, | ||
| psdOutput : psd, | ||
| blackBoxRate : this._blackBoxRate, | ||
| minimum: min, | ||
| maximum: max, | ||
| }; | ||
| return psdData; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safeguards & make Welch parameters configurable
dataLoadFrequencyPSD uses fixed values (points_per_segment = 512, overlap_count = 256) and proceeds even when the captured sample-count is smaller than points_per_segment.
If samples.length < 512, _fft_segmented returns an empty array, which later causes segmentsCount === 0 and a division-by-zero in _psd (lines 556-572).
Consider:
-const points_per_segment = 512,
- overlap_count = 256;
+// Pick reasonable defaults, but clamp them to the available data
+const MIN_SEGMENT = 64; // avoid pathological tiny segments
+let points_per_segment = 512;
+let overlap_count = 256;
+
+// Shrink segment size when we have fewer samples than the default
+if (flightSamples.samples.length < points_per_segment) {
+ points_per_segment = Math.max(
+ MIN_SEGMENT,
+ 1 << Math.floor(Math.log2(flightSamples.samples.length))
+ );
+ overlap_count = Math.floor(points_per_segment / 2);
+}
+
+// Bail out early when we still do not have enough data
+if (flightSamples.samples.length < points_per_segment) {
+ return {
+ error: 'Not enough data for PSD calculation',
+ fieldName: this._dataBuffer.fieldName
+ };
+}Exposing points_per_segment & overlap_count through the UI (or at least constants in userSettings) will also address the feedback about “too small number of points per segment / too large overlap”.
Committable suggestion skipped: line range outside the PR's diff.
| for (let i = 0; i < dataCount; i++) { | ||
| psdOutput[i] = 0.0; | ||
| for (let j = 0; j < segmentsCount; j++) { | ||
| let p = scale * fftOutput[j][i] ** 2; | ||
| if (dataCount % 2) { | ||
| p *= 2; | ||
| } else if (i != dataCount - 1) { | ||
| p *= 2; | ||
| } | ||
| psdOutput[i] += p; | ||
| } | ||
|
|
||
| const min_avg = 1e-5; // limit min value for -50db | ||
| let avg = psdOutput[i] / segmentsCount; | ||
| avg = Math.max(avg, min_avg); | ||
| psdOutput[i] = 10 * Math.log10(avg); | ||
| } |
There was a problem hiding this comment.
Incorrect power doubling logic & potential divide-by-zero
-
segmentsCountmay be0(see previous comment) – dividing by it (line 569) will yieldInfinity / NaN.
Guard against emptyfftOutput. -
Doubling of the one-sided spectrum should exclude the DC component (
i === 0) and, for evenn_per_seg, the Nyquist bin (i === dataCount/2).
Current checks (dataCount % 2 …/i != dataCount - 1) are wrong for a real-input FFT.
- for (let i = 0; i < dataCount; i++) {
+ if (segmentsCount === 0) return psdOutput.fill(NaN); // early-exit guard
+
+ const nyquistIdx = (dataCount % 2 === 0) ? dataCount / 2 : null;
+ for (let i = 0; i < dataCount; i++) {
...
- if (dataCount % 2) {
- p *= 2;
- } else if (i != dataCount - 1) {
- p *= 2;
- }
+ // Double all bins except DC and Nyquist (if present)
+ if (i !== 0 && i !== nyquistIdx) {
+ p *= 2;
+ }
...
}This follows the same rule used by SciPy’s signal.welch.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (let i = 0; i < dataCount; i++) { | |
| psdOutput[i] = 0.0; | |
| for (let j = 0; j < segmentsCount; j++) { | |
| let p = scale * fftOutput[j][i] ** 2; | |
| if (dataCount % 2) { | |
| p *= 2; | |
| } else if (i != dataCount - 1) { | |
| p *= 2; | |
| } | |
| psdOutput[i] += p; | |
| } | |
| const min_avg = 1e-5; // limit min value for -50db | |
| let avg = psdOutput[i] / segmentsCount; | |
| avg = Math.max(avg, min_avg); | |
| psdOutput[i] = 10 * Math.log10(avg); | |
| } | |
| // early-exit guard against divide-by-zero | |
| if (segmentsCount === 0) return psdOutput.fill(NaN); | |
| const nyquistIdx = (dataCount % 2 === 0) ? dataCount / 2 : null; | |
| for (let i = 0; i < dataCount; i++) { | |
| psdOutput[i] = 0.0; | |
| for (let j = 0; j < segmentsCount; j++) { | |
| let p = scale * fftOutput[j][i] ** 2; | |
| // Double all bins except DC and Nyquist (if present) | |
| if (i !== 0 && i !== nyquistIdx) { | |
| p *= 2; | |
| } | |
| psdOutput[i] += p; | |
| } | |
| const min_avg = 1e-5; // limit min value for -50 dB | |
| let avg = psdOutput[i] / segmentsCount; | |
| avg = Math.max(avg, min_avg); | |
| psdOutput[i] = 10 * Math.log10(avg); | |
| } |
src/graph_spectrum_plot.js
Outdated
| GraphSpectrumPlot._drawFrequencyPSDGraph = function (canvasCtx) { | ||
| const HEIGHT = canvasCtx.canvas.height - MARGIN; | ||
| const WIDTH = canvasCtx.canvas.width; | ||
| const LEFT = canvasCtx.canvas.left; | ||
| const TOP = canvasCtx.canvas.top; | ||
|
|
||
| const PLOTTED_BLACKBOX_RATE = this._fftData.blackBoxRate / this._zoomX; | ||
|
|
||
| canvasCtx.save(); | ||
| canvasCtx.translate(LEFT, TOP); | ||
| this._drawGradientBackground(canvasCtx, WIDTH, HEIGHT); | ||
|
|
||
| const pointsCount = this._fftData.psdLength; | ||
| const scaleX = 2 * WIDTH / PLOTTED_BLACKBOX_RATE * this._zoomX; | ||
| canvasCtx.beginPath(); | ||
| canvasCtx.lineWidth = 1; | ||
| canvasCtx.strokeStyle = "orange"; | ||
|
|
||
| canvasCtx.moveTo(0, 0); | ||
| const a1 = Math.abs(this._fftData.minimum), | ||
| a2 = Math.abs(this._fftData.maximum), | ||
| limit = Math.max(a1, a2); | ||
| const scaleY = HEIGHT / 2 / limit; | ||
| for (let pointNum = 0; pointNum < pointsCount; pointNum += 2) { | ||
| const freq = PLOTTED_BLACKBOX_RATE / 2 * pointNum / pointsCount; | ||
| const y = HEIGHT / 2 - this._fftData.psdOutput[pointNum] * scaleY; | ||
| canvasCtx.lineTo(freq * scaleX, y); | ||
| } | ||
| canvasCtx.stroke(); | ||
|
|
||
| canvasCtx.restore(); | ||
|
|
||
| this._drawAxisLabel( | ||
| canvasCtx, | ||
| this._fftData.fieldName, | ||
| WIDTH - 4, | ||
| HEIGHT - 6, | ||
| "right", | ||
| ); | ||
| this._drawHorizontalGridLines( | ||
| canvasCtx, | ||
| PLOTTED_BLACKBOX_RATE / 2, | ||
| LEFT, | ||
| TOP, | ||
| WIDTH, | ||
| HEIGHT, | ||
| MARGIN, | ||
| "Hz", | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PSD graph drawing implementation needs validation and improved mouse interaction.
The basic implementation looks good, but there are a few improvements that could be made:
- Missing validation for
this._fftData.psdLength,this._fftData.psdOutput,this._fftData.minimum, andthis._fftData.maximum - Using
pointNum += 2without a comment explaining why only every other point is plotted - No support for showing imported spectrums (unlike the frequency graph)
- No support for mouse position tracking in
_drawMousePositionfunction for this spectrum type
I recommend adding data validation to prevent errors when properties are missing:
GraphSpectrumPlot._drawFrequencyPSDGraph = function (canvasCtx) {
const HEIGHT = canvasCtx.canvas.height - MARGIN;
const WIDTH = canvasCtx.canvas.width;
const LEFT = canvasCtx.canvas.left;
const TOP = canvasCtx.canvas.top;
const PLOTTED_BLACKBOX_RATE = this._fftData.blackBoxRate / this._zoomX;
canvasCtx.save();
canvasCtx.translate(LEFT, TOP);
this._drawGradientBackground(canvasCtx, WIDTH, HEIGHT);
+ // Validate required properties
+ if (!this._fftData.psdLength || !this._fftData.psdOutput ||
+ this._fftData.minimum === undefined || this._fftData.maximum === undefined) {
+ console.error('PSD data is missing required properties');
+ canvasCtx.restore();
+ return;
+ }
+
const pointsCount = this._fftData.psdLength;
const scaleX = 2 * WIDTH / PLOTTED_BLACKBOX_RATE * this._zoomX;
canvasCtx.beginPath();
canvasCtx.lineWidth = 1;
canvasCtx.strokeStyle = "orange";
canvasCtx.moveTo(0, 0);
const a1 = Math.abs(this._fftData.minimum),
a2 = Math.abs(this._fftData.maximum),
limit = Math.max(a1, a2);
const scaleY = HEIGHT / 2 / limit;
+ // Draw PSD curve - only plot every other point for performance optimization
for (let pointNum = 0; pointNum < pointsCount; pointNum += 2) {
const freq = PLOTTED_BLACKBOX_RATE / 2 * pointNum / pointsCount;
const y = HEIGHT / 2 - this._fftData.psdOutput[pointNum] * scaleY;
canvasCtx.lineTo(freq * scaleX, y);
}
canvasCtx.stroke();Also, mouse position tracking should be updated in the _drawMousePosition function (around line 1404):
// X axis
if (
this._spectrumType === SPECTRUM_TYPE.FREQUENCY ||
this._spectrumType === SPECTRUM_TYPE.FREQ_VS_THROTTLE ||
- this._spectrumType === SPECTRUM_TYPE.FREQ_VS_RPM
+ this._spectrumType === SPECTRUM_TYPE.FREQ_VS_RPM ||
+ this._spectrumType === SPECTRUM_TYPE.PSD_VS_FREQUENCY
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GraphSpectrumPlot._drawFrequencyPSDGraph = function (canvasCtx) { | |
| const HEIGHT = canvasCtx.canvas.height - MARGIN; | |
| const WIDTH = canvasCtx.canvas.width; | |
| const LEFT = canvasCtx.canvas.left; | |
| const TOP = canvasCtx.canvas.top; | |
| const PLOTTED_BLACKBOX_RATE = this._fftData.blackBoxRate / this._zoomX; | |
| canvasCtx.save(); | |
| canvasCtx.translate(LEFT, TOP); | |
| this._drawGradientBackground(canvasCtx, WIDTH, HEIGHT); | |
| const pointsCount = this._fftData.psdLength; | |
| const scaleX = 2 * WIDTH / PLOTTED_BLACKBOX_RATE * this._zoomX; | |
| canvasCtx.beginPath(); | |
| canvasCtx.lineWidth = 1; | |
| canvasCtx.strokeStyle = "orange"; | |
| canvasCtx.moveTo(0, 0); | |
| const a1 = Math.abs(this._fftData.minimum), | |
| a2 = Math.abs(this._fftData.maximum), | |
| limit = Math.max(a1, a2); | |
| const scaleY = HEIGHT / 2 / limit; | |
| for (let pointNum = 0; pointNum < pointsCount; pointNum += 2) { | |
| const freq = PLOTTED_BLACKBOX_RATE / 2 * pointNum / pointsCount; | |
| const y = HEIGHT / 2 - this._fftData.psdOutput[pointNum] * scaleY; | |
| canvasCtx.lineTo(freq * scaleX, y); | |
| } | |
| canvasCtx.stroke(); | |
| canvasCtx.restore(); | |
| this._drawAxisLabel( | |
| canvasCtx, | |
| this._fftData.fieldName, | |
| WIDTH - 4, | |
| HEIGHT - 6, | |
| "right", | |
| ); | |
| this._drawHorizontalGridLines( | |
| canvasCtx, | |
| PLOTTED_BLACKBOX_RATE / 2, | |
| LEFT, | |
| TOP, | |
| WIDTH, | |
| HEIGHT, | |
| MARGIN, | |
| "Hz", | |
| ); | |
| GraphSpectrumPlot._drawFrequencyPSDGraph = function (canvasCtx) { | |
| const HEIGHT = canvasCtx.canvas.height - MARGIN; | |
| const WIDTH = canvasCtx.canvas.width; | |
| const LEFT = canvasCtx.canvas.left; | |
| const TOP = canvasCtx.canvas.top; | |
| const PLOTTED_BLACKBOX_RATE = this._fftData.blackBoxRate / this._zoomX; | |
| canvasCtx.save(); | |
| canvasCtx.translate(LEFT, TOP); | |
| this._drawGradientBackground(canvasCtx, WIDTH, HEIGHT); | |
| // Validate required properties | |
| if (!this._fftData.psdLength || | |
| !this._fftData.psdOutput || | |
| this._fftData.minimum === undefined || | |
| this._fftData.maximum === undefined) { | |
| console.error('PSD data is missing required properties'); | |
| canvasCtx.restore(); | |
| return; | |
| } | |
| const pointsCount = this._fftData.psdLength; | |
| const scaleX = 2 * WIDTH / PLOTTED_BLACKBOX_RATE * this._zoomX; | |
| canvasCtx.beginPath(); | |
| canvasCtx.lineWidth = 1; | |
| canvasCtx.strokeStyle = "orange"; | |
| canvasCtx.moveTo(0, 0); | |
| const a1 = Math.abs(this._fftData.minimum), | |
| a2 = Math.abs(this._fftData.maximum), | |
| limit = Math.max(a1, a2); | |
| const scaleY = HEIGHT / 2 / limit; | |
| // Draw PSD curve - only plot every other point for performance optimization | |
| for (let pointNum = 0; pointNum < pointsCount; pointNum += 2) { | |
| const freq = PLOTTED_BLACKBOX_RATE / 2 * pointNum / pointsCount; | |
| const y = HEIGHT / 2 - this._fftData.psdOutput[pointNum] * scaleY; | |
| canvasCtx.lineTo(freq * scaleX, y); | |
| } | |
| canvasCtx.stroke(); | |
| canvasCtx.restore(); | |
| this._drawAxisLabel( | |
| canvasCtx, | |
| this._fftData.fieldName, | |
| WIDTH - 4, | |
| HEIGHT - 6, | |
| "right", | |
| ); | |
| this._drawHorizontalGridLines( | |
| canvasCtx, | |
| PLOTTED_BLACKBOX_RATE / 2, | |
| LEFT, | |
| TOP, | |
| WIDTH, | |
| HEIGHT, | |
| MARGIN, | |
| "Hz", | |
| ); | |
| }; |
|
A small question about FFT algorithm. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/graph_spectrum_calc.js (1)
309-323:⚠️ Potential issueAdd null check to prevent potential crash.
The current implementation can throw an error if
this._dataBuffer.curveis null whenscaledis true. This was previously flagged in a code review but hasn't been fixed in this implementation.Apply this fix:
if (scaled) { + if (!this._dataBuffer.curve) { + samples[samplesCount] = frame[this._dataBuffer.fieldIndex]; + } else { samples[samplesCount] = (this._dataBuffer.curve.lookupRaw(frame[this._dataBuffer.fieldIndex])); + } } else { samples[samplesCount] = frame[this._dataBuffer.fieldIndex]; }
♻️ Duplicate comments (3)
src/graph_spectrum_calc.js (3)
109-131: 🛠️ Refactor suggestionAdd safeguards and make Welch parameters configurable.
The method uses fixed values for
points_per_segmentandoverlap_countwhich may not work well for all data sizes. IfflightSamples.samples.length < 512, this will cause issues when calculating PSD.Apply this improvement:
-const points_per_segment = 512, - overlap_count = 256; +// Pick reasonable defaults, but clamp them to the available data +const MIN_SEGMENT = 64; // avoid pathological tiny segments +let points_per_segment = 512; +let overlap_count = 256; + +// Shrink segment size when we have fewer samples than the default +if (flightSamples.samples.length < points_per_segment) { + points_per_segment = Math.max( + MIN_SEGMENT, + 1 << Math.floor(Math.log2(flightSamples.samples.length)) + ); + overlap_count = Math.floor(points_per_segment / 2); +} + +// Bail out early when we still do not have enough data +if (flightSamples.samples.length < points_per_segment) { + return { + error: 'Not enough data for PSD calculation', + fieldName: this._dataBuffer.fieldName + }; +}
516-573:⚠️ Potential issueFix power doubling logic and add division-by-zero protection.
The current implementation has two issues:
- Potential division-by-zero error when
segmentsCountis 0- Incorrect power doubling logic for real-input FFT
Apply this fix:
// Compute average for scaled power + if (segmentsCount === 0) return psdOutput.fill(NaN); // early-exit guard + const nyquistIdx = (dataCount % 2 === 0) ? dataCount / 2 : null; for (let i = 0; i < dataCount; i++) { psdOutput[i] = 0.0; for (let j = 0; j < segmentsCount; j++) { let p = scale * fftOutput[j][i] ** 2; - if (i != dataCount - 1) { - p *= 2; - } + // Double all bins except DC and Nyquist (if present) + if (i !== 0 && i !== nyquistIdx) { + p *= 2; + } psdOutput[i] += p; }
579-594:⚠️ Potential issueFix FFT segment processing to use complex magnitude and include the last segment.
There are two issues with the current implementation:
- It ignores the imaginary part of the FFT output, using only the real part
- The loop condition misses the last valid segment due to a strict inequality
Apply this fix:
-for (let i = 0; i < samplesCount - n_per_seg; i += n_per_seg - n_overlap) { +for (let i = 0; i <= samplesCount - n_per_seg; i += n_per_seg - n_overlap) { const fftInput = samples.slice(i, i + n_per_seg); if (userSettings.analyserHanning) { this._hanningWindow(fftInput, n_per_seg); } - const fftOutput = this._fft(fftInput); - output.push(fftOutput.slice(0, n_per_seg)); + const fftComplex = this._fft(fftInput); // [re0, im0, re1, im1, ...] + const mag = new Float64Array(n_per_seg); + for (let k = 0; k < n_per_seg; k++) { + const re = fftComplex[2 * k]; + const im = fftComplex[2 * k + 1]; + mag[k] = Math.hypot(re, im); // √(re² + im²) + } + output.push(mag); }
🧹 Nitpick comments (1)
src/graph_spectrum_calc.js (1)
516-519: Document PSD function parameters more clearly.The function documentation could be improved to explain what each parameter does, especially the scaling parameter which has two possible values.
/** - * Compute PSD for data samples by Welch method follow Python code + * Compute Power Spectral Density (PSD) for data samples using Welch's method + * @param {Float64Array} samples - Input time-domain samples + * @param {number} fs - Sampling frequency in Hz + * @param {number} n_per_seg - Number of points per segment + * @param {number} n_overlap - Number of points to overlap between segments + * @param {string} scaling - Scaling mode: 'density' (dB/Hz) or 'spectrum' (dB) + * @returns {Float64Array} - PSD values in dB */
The clever coderabbitai doubt too, it suggests: But i confuse a small bit, how spectrum worked before?... |
|
I've tryed follow AI suggestion and get two side symetrical spectrum! Seems i understand what is our FFT result now! |
|
IMO it should work like last commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/graph_spectrum_calc.js (1)
309-324:⚠️ Potential issueAdd null check for curve before using lookupRaw
When
scaled === true, the code directly callsthis._dataBuffer.curve.lookupRaw(...)without checking ifcurveexists first. This could cause a null/undefined reference error.- if (scaled) { + if (scaled && this._dataBuffer.curve) { samples[samplesCount] = (this._dataBuffer.curve.lookupRaw(frame[this._dataBuffer.fieldIndex])); } else { samples[samplesCount] = frame[this._dataBuffer.fieldIndex]; }
♻️ Duplicate comments (3)
src/graph_spectrum_calc.js (3)
109-131: 🛠️ Refactor suggestionAdd configurable PSD parameters
The current implementation uses fixed values for segment size (512) and overlap (256), which may not be optimal for all datasets or provide sufficient smoothing. As mentioned in the PR comments, these parameters significantly affect smoothing quality.
-const points_per_segment = 512, - overlap_count = 256; +// Use configurable parameters with reasonable defaults +const points_per_segment = userSettings.psdPointsPerSegment || 512, + overlap_count = userSettings.psdOverlapCount || 256; + +// Ensure we have enough data for PSD calculation +if (flightSamples.samples.length < points_per_segment) { + return { + error: 'Not enough data for PSD calculation', + fieldName: this._dataBuffer.fieldName + }; +}
575-593: 🛠️ Refactor suggestionFix segment processing logic
There are two issues in the FFT segmentation function:
- The loop condition
i < samplesCount - n_per_segskips the last valid segment. It should bei <= samplesCount - n_per_seg.- The function only uses half of the FFT output (magnitudes up to n_per_seg/2) which is correct for real data, but ensure n_per_seg is always even to avoid issues.
GraphSpectrumCalc._fft_segmented = function(samples, n_per_seg, n_overlap) { const samplesCount = samples.length; let output = []; - for (let i = 0; i < samplesCount - n_per_seg; i += n_per_seg - n_overlap) { + // Ensure we capture all valid segments including the last one + for (let i = 0; i <= samplesCount - n_per_seg; i += n_per_seg - n_overlap) { const fftInput = samples.slice(i, i + n_per_seg); if (userSettings.analyserHanning) { this._hanningWindow(fftInput, n_per_seg); } const fftComplex = this._fft(fftInput); + // Ensure n_per_seg is even to avoid truncation issues const magnitudes = new Float64Array(n_per_seg / 2); for (let i = 0; i < n_per_seg / 2; i++) { const re = fftComplex[2 * i];
519-569:⚠️ Potential issueFix power doubling logic and add division-by-zero check
There are two issues with the PSD calculation:
- The code incorrectly applies power doubling to all frequencies except the last one. For real signals, doubling should exclude both the DC component (i=0) and Nyquist frequency (if present).
- There's no check for empty segments, which could lead to division by zero.
GraphSpectrumCalc._psd = function(samples, fs, n_per_seg, n_overlap, scaling = 'density') { // Compute FFT for samples segments const fftOutput = this._fft_segmented(samples, n_per_seg, n_overlap); const dataCount = fftOutput[0]?.length || 0; const segmentsCount = fftOutput.length; + + // Check if we have any data to process + if (segmentsCount === 0 || dataCount === 0) { + return new Float64Array(0); + } + const psdOutput = new Float64Array(dataCount); // Compute power scale coef let scale = 1; if (userSettings.analyserHanning) { // ... [scale calculation code remains unchanged] } // Compute average for scaled power for (let i = 0; i < dataCount; i++) { psdOutput[i] = 0.0; for (let j = 0; j < segmentsCount; j++) { let p = scale * fftOutput[j][i] ** 2; - if (i != dataCount - 1) { + // Double all bins except DC (i=0) and Nyquist (if present at dataCount/2) + const isNyquist = (dataCount % 2 === 0) && (i === dataCount / 2); + if (i !== 0 && !isNyquist) { p *= 2; } psdOutput[i] += p; }
🧹 Nitpick comments (1)
src/graph_spectrum_calc.js (1)
516-569: Consider making scaling mode configurableThe
_psdfunction supports two scaling modes ('density' and 'spectrum'), but only 'density' is used in the calling function. Consider making this configurable via user settings to offer more flexibility.GraphSpectrumCalc.dataLoadFrequencyPSD = function() { const points_per_segment = 512, overlap_count = 256; const flightSamples = this._getFlightSamplesFreq(false); - const psd = this._psd(flightSamples.samples, this._blackBoxRate, points_per_segment, overlap_count); + const scaling = userSettings.psdScalingMode || 'density'; // Add to userSettings + const psd = this._psd(flightSamples.samples, this._blackBoxRate, points_per_segment, overlap_count, scaling);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/graph_spectrum_calc.js(4 hunks)
🔇 Additional comments (1)
src/graph_spectrum_calc.js (1)
587-591: Good implementation of complex magnitude calculationThe code correctly computes the magnitude of complex FFT results using the Euclidean norm (hypot) of real and imaginary parts. This is the right approach for spectral analysis.
Co-authored-by: Mark Haslinghuis <[email protected]>
Co-authored-by: Mark Haslinghuis <[email protected]>
|
|
@haslinghuis |
…taflight#820)" This reverts commit a263152.
|
@demvlad you don't have access to revert button ?
|
|
@nerdCopter please revert - so @demvlad can start over ? |
I have access. It creates new PR. Can i do this? |
|
Yes - simple revert. And start over. Or better fix current code in new PR. |
@haslinghuis @nerdCopter |
|
@demvlad reverted. |
… of #820 PR) (#826) * Added computing of PSD - Power Spectral Density for spectrum chart * Added PSD curves at spectrum chart * Removed redundant check TODO code * Code issues are resolved * PSD caption is edited in index.html Co-authored-by: MikeNomatter <[email protected]> * Removed non execute condition. The FFT output is allways even value * Powers scale computing code refactoring * Draw the all points of PSD data * Resolved issue of magnitude computing * Added vertical grid lines * Added grid with db value captions * The PSD Y axises range is alligned by value 10db/Hz * Added setup points per segment by using vertical slider (Y-scale slider) * maximal num per segment count value is limited by data samples count * The names variable are changed to JS code style * Added the maximal noise frequency label * The 'Max motor noise' label is renamed to 'Max noise' * Added mouse marker at the PSD chart * Zeroes frequency magnitudes removed from curves min-max range * Decreased spectrums charts plot transparent * Code style improvement * Max noise definition low frequency limit is changed from 100Hz to 50Hz for PSD chart * Added horizont mouse position marker line at PSD chart * The max noise label is shifted down at PSD chart * The PSD chart updates immediately after Y scale sliders change * PSD unit label is changed at dBm/Hz * The minimum PSD value is set as -70dBm * Code style improvement Co-authored-by: Mark Haslinghuis <[email protected]> * Code style improvement Co-authored-by: Mark Haslinghuis <[email protected]> * Code style improvement * Added checking for missing samples segment data Co-authored-by: Mark Haslinghuis <[email protected]> * Code style improvement Co-authored-by: Mark Haslinghuis <[email protected]> * Resolved wrong variable name * Resolved missing coma * Resolved "for" loop condition issue, when num per segment value is equal samples count * Code style improvement --------- Co-authored-by: MikeNomatter <[email protected]> Co-authored-by: Mark Haslinghuis <[email protected]>
4 participants
The power spectral density (PSD) is usefull tools for noise analysis.
The PID tool box shows PSD at its chart.
This PR starts this work.
Added power spectral density curves at the spectrum chart.
I try to implement Welch PSD computing method follow python SciPy library and Python Spectral Analysis tutorial.
The Blackbox explorer PSD chart for this PR (in db/Hz unit):
The PID tool box PSD chart:
The Python PSD chart:
The PSD curves are showed in db/Hz units.
The Welch method can compute signal power mean average values for many data samples segments with overlapping.
The mean average gives smooth spectrum curves.
My future plans:
Its exists two scale mode also: "density" and "spectrum" in Welch method. I use "density" on the example above. Interested to look at "spectrum" mode too.
The current BBExplorer amplitude (not PSD) spectrum shows some internal 0...100 units, it depends from curves scale. It has much pulsations, therefore it unpossible to show several spectrums at the chart without some filtering. The PSD has standard db/Hz units. It can do smooth by using standard settings for good view.
The current BBExplorers Frequency spectrum:
Summary by CodeRabbit