The simple PSD spectrum comparison of one log file data #847
Conversation
## Walkthrough
This change refactors the `ImportedCurves` module to encapsulate curve data privately and adds methods for curve access and mutation. It introduces functionality to add the current power spectral density (PSD) spectrum as an imported curve, updates legend rendering, and adds UI and keyboard shortcuts to trigger spectrum import. Additionally, it enhances spectrum plotting and analyser toggling logic to support multi-spectrum handling and Ctrl-key modifiers.
## Changes
| File(s) | Change Summary |
|-----------------------------|---------------------------------------------------------------------------------------------------------------|
| src/graph_imported_curves.js | Encapsulated `_curvesData` as private; added `getCurve(index)`, `addCurve(points, name)`, `isNewCurve(name)`, `removeCurve(name)`, `removeAllCurves()`, `isFull()`, and `isEmpty()` methods; improved CSV parsing with trimming; increased max import count from 5 to 6. |
| src/graph_spectrum.js | Added `prepareSpectrumForComparison()`, `shouldAddCurrentSpectrumBeforeReload()`, and `isMultiSpectrum()` methods; updated `plotSpectrum` to conditionally add spectrum to import and manage reload logic; toggled "Add" button visibility by spectrum type. |
| src/graph_spectrum_plot.js | Added `getCurveColor(index)`, `addCurrentSpectrumIntoImport()`, `isNewComparedCurve(name)`, `removeComparedCurve(name)`, `isMultiSpectrum()`, and `isImportedCurvesMaxCount()` methods; replaced direct curve data access with `getCurve()` calls; updated legend rendering and curve color usage; added logic to add current PSD spectrum on import if none present; improved drawing and legend text alignment. |
| src/main.js | Modified `onLegendSelectionChange` to accept `ctrlKey`; updated analyser toggle logic to respect Ctrl key; minor whitespace fixes in video export dialog callback. |
| index.html | Shortened spectrum comparison button labels ("Export" to "Exp", "Import" to "Imp", "Clear" to "Clr"); no new buttons added. |
| src/graph_legend.js | Modified legend selection callback to pass Ctrl key state along with selection change status. |
| src/grapher.js | Updated `setDrawAnalyser` to accept `ctrlKey` parameter and call `prepareSpectrumForComparison()` accordingly; added `hasMultiSpectrumAnalyser()` method. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant MainJS
participant FlightLogAnalyser
participant GraphSpectrumPlot
participant ImportedCurves
User->>MainJS: Press Insert key or click "Add" button
MainJS->>FlightLogAnalyser: prepareSpectrumForComparison()
FlightLogAnalyser->>GraphSpectrumPlot: addCurrentSpectrumIntoImport()
GraphSpectrumPlot->>ImportedCurves: addCurve(points, name) (if new PSD curve)
ImportedCurves-->>GraphSpectrumPlot: Curve added and callback triggeredEstimated code review effort3 (30–60 minutes) Possibly related PRs
Suggested reviewers
Learnt from: demvlad Learnt from: demvlad Learnt from: nerdCopter Learnt from: demvlad Learnt from: demvlad Learnt from: demvlad Learnt from: demvlad Learnt from: nerdCopter Learnt from: demvlad Learnt from: demvlad |
![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
🔭 Outside diff range comments (1)
src/graph_spectrum_plot.js (1)
264-298: Improve the smoothing algorithm implementation.The current moving average implementation has several issues:
- Fixed filter size of 200 points may be too large for curves with fewer points
- Inefficient recalculation of window bounds for each point
- The average is plotted at the current point rather than being properly centered
Consider this improved implementation:
- const filterPointsCount = 200; + const filterPointsCount = Math.min(200, Math.floor(pointsCount * 0.1)); // Use 10% of points or 200, whichever is smaller + const halfWindow = Math.floor(filterPointsCount / 2); + + // Pre-calculate cumulative sum for efficient moving average + const cumulativeSum = [0]; + for (let i = 0; i < pointsCount; i++) { + cumulativeSum[i + 1] = cumulativeSum[i] + curvesPonts[i].y; + } + for (let pointNum = 0; pointNum < pointsCount; pointNum++) { - // Apply moving average filter at spectrum points to get visible line - let filterStartPoint = pointNum - filterPointsCount / 2; - if (filterStartPoint < 0) { - filterStartPoint = 0; - } - let filterStopPoint = filterStartPoint + filterPointsCount; - if (filterStopPoint > pointsCount) { - filterStopPoint = pointsCount; - filterStartPoint = filterStopPoint - filterPointsCount; - if (filterStartPoint < 0) { - filterStartPoint = 0; - } - } - let middleValue = 0; - for (let i = filterStartPoint; i < filterStopPoint; i++) { - middleValue += curvesPonts[i].y; - } - middleValue /= filterPointsCount; + const filterStartPoint = Math.max(0, pointNum - halfWindow); + const filterStopPoint = Math.min(pointsCount, pointNum + halfWindow + 1); + const actualWindowSize = filterStopPoint - filterStartPoint; + const middleValue = (cumulativeSum[filterStopPoint] - cumulativeSum[filterStartPoint]) / actualWindowSize; canvasCtx.lineTo(curvesPonts[pointNum].x * scaleX, HEIGHT - middleValue * fftScale); }
🧹 Nitpick comments (3)
src/graph_imported_curves.js (1)
33-33: Consider browser compatibility for Array.at() method.The
at(-1)method is relatively new and may not be supported in older browsers. Consider usingstringRows[stringRows.length - 1]for better compatibility.-if (stringRows.at(-1) == "") { +if (stringRows[stringRows.length - 1] == "") {src/graph_spectrum.js (1)
428-439: Consider handling all spectrum types for consistent filename generation.The method correctly handles FREQUENCY and POWER_SPECTRAL_DENSITY types, but doesn't provide suffixes for other spectrum types (FREQ_VS_THROTTLE, FREQ_VS_RPM, etc.).
Consider adding a default case or explicit handling for other spectrum types:
this.getExportedFileName = function() { let fileName = $(".log-filename").text().split(".")[0]; switch (userSettings.spectrumType) { case SPECTRUM_TYPE.FREQUENCY: fileName = fileName + "_sp"; break; case SPECTRUM_TYPE.POWER_SPECTRAL_DENSITY: fileName = fileName + "_psd"; break; + case SPECTRUM_TYPE.FREQ_VS_THROTTLE: + fileName = fileName + "_freq_throttle"; + break; + case SPECTRUM_TYPE.FREQ_VS_RPM: + fileName = fileName + "_freq_rpm"; + break; + case SPECTRUM_TYPE.PSD_VS_THROTTLE: + fileName = fileName + "_psd_throttle"; + break; + case SPECTRUM_TYPE.PSD_VS_RPM: + fileName = fileName + "_psd_rpm"; + break; + case SPECTRUM_TYPE.PIDERROR_VS_SETPOINT: + fileName = fileName + "_piderror"; + break; } return fileName; };src/graph_spectrum_plot.js (1)
1604-1644: Good enhancement to mouse position handling for PSD spectrums.The differentiation between single PSD spectrum and multiple imported curves is well implemented. The addition of PSD heatmap value display improves user experience.
Consider extracting the complex condition for better readability:
- if (this._spectrumType === SPECTRUM_TYPE.POWER_SPECTRAL_DENSITY && this._importedPSD.curvesCount() == 0) { // single PSD spectrum + const isSinglePSDSpectrum = this._spectrumType === SPECTRUM_TYPE.POWER_SPECTRAL_DENSITY && this._importedPSD.curvesCount() == 0; + if (isSinglePSDSpectrum) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
index.html(1 hunks)public/js/webworkers/spectrum-export-worker.js(1 hunks)src/graph_imported_curves.js(1 hunks)src/graph_spectrum.js(2 hunks)src/graph_spectrum_calc.js(1 hunks)src/graph_spectrum_plot.js(38 hunks)src/main.js(4 hunks)src/user_settings_dialog.js(3 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
Learnt from: nerdCopter
PR: betaflight/blackbox-log-viewer#822
File: .github/workflows/deploy-preview.yml:17-19
Timestamp: 2025-05-14T16:08:27.254Z
Learning: For betaflight/blackbox-log-viewer repository, the team is aware of the security implications of using `pull_request_target` without specifying `persist-credentials: false` in the checkout action, and has chosen to proceed with the current approach. This concern should not be flagged in future reviews.
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.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#827
File: src/graph_spectrum.js:200-207
Timestamp: 2025-05-10T19:47:41.430Z
Learning: The vertical zoom slider recalculation in the spectrum analyzer is designed to work only for POWER_SPECTRAL_DENSITY spectrum type, not for PSD_VS_THROTTLE or PSD_VS_RPM types.
src/user_settings_dialog.js (1)
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
src/graph_spectrum_calc.js (2)
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.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
public/js/webworkers/spectrum-export-worker.js (5)
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.
Learnt from: nerdCopter
PR: betaflight/blackbox-log-viewer#829
File: src/graph_spectrum_calc.js:332-335
Timestamp: 2025-05-12T19:49:35.667Z
Learning: In the Blackbox Log Viewer, the `_getFlightSamplesFreq()` function should return a sliced array `samples.slice(0, samplesCount)` rather than the full preallocated array, as code that consumes this function expects the array length to match the count of valid samples.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
Learnt from: Quick-Flash
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-05-13T17:48:20.477Z
Learning: When using FFT (Fast Fourier Transform), enforcing power-of-2 input lengths improves computational efficiency and numerical stability but can result in discarding up to almost 50% of the flight data in worst-case scenarios.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-05-14T08:52:53.032Z
Learning: The FFT.complex library in Betaflight Blackbox Log Viewer provides two main methods: simple(output, input, type) and process(output, outputOffset, outputStride, input, inputOffset, inputStride, type). The 'simple' method is the one currently used in the codebase, while 'process' offers more granular control over memory layout with offsets and strides.
src/main.js (3)
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.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
Learnt from: nerdCopter
PR: betaflight/blackbox-log-viewer#829
File: src/graph_spectrum_calc.js:332-335
Timestamp: 2025-05-12T19:49:35.667Z
Learning: In the Blackbox Log Viewer, the `_getFlightSamplesFreq()` function should return a sliced array `samples.slice(0, samplesCount)` rather than the full preallocated array, as code that consumes this function expects the array length to match the count of valid samples.
src/graph_imported_curves.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.js (2)
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.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
src/graph_spectrum_plot.js (4)
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.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#827
File: src/graph_spectrum.js:200-207
Timestamp: 2025-05-10T19:47:41.430Z
Learning: The vertical zoom slider recalculation in the spectrum analyzer is designed to work only for POWER_SPECTRAL_DENSITY spectrum type, not for PSD_VS_THROTTLE or PSD_VS_RPM types.
Learnt from: nerdCopter
PR: betaflight/blackbox-log-viewer#829
File: src/graph_spectrum_calc.js:332-335
Timestamp: 2025-05-12T19:49:35.667Z
Learning: In the Blackbox Log Viewer, the `_getFlightSamplesFreq()` function should return a sliced array `samples.slice(0, samplesCount)` rather than the full preallocated array, as code that consumes this function expects the array length to match the count of valid samples.
🧬 Code Graph Analysis (2)
src/main.js (1)
src/graph_spectrum.js (1)
fileName(429-429)
src/graph_spectrum_plot.js (1)
src/graph_imported_curves.js (1)
ImportedCurves(1-98)
🪛 ESLint
src/graph_imported_curves.js
[error] 79-80: Missing semicolon.
(semi)
[error] 88-89: Missing semicolon.
(semi)
🔇 Additional comments (19)
src/graph_spectrum_calc.js (1)
133-134: Naming consistency: rename remainingpsdOutputproperty tofftOutputOne return object in
src/graph_spectrum_calc.jsstill exposes the old property name. Update it so that every PSD‐related function returnsfftLengthandfftOutput.• File: src/graph_spectrum_calc.js
– Locate the return block that currently reads:
js fftLength: psdLength, psdOutput: psdOutput,
– Change to:
diff fftLength: psdLength, - psdOutput: psdOutput, + fftOutput: psdOutput,After this change, re-run:
rg "psdLength|psdOutput" --type js src/to confirm no old property names remain.
⛔ Skipped due to learnings
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.Learnt from: demvlad PR: betaflight/blackbox-log-viewer#0 File: :0-0 Timestamp: 2025-06-29T18:40:50.980Z Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.Learnt from: nerdCopter PR: betaflight/blackbox-log-viewer#829 File: src/graph_spectrum_calc.js:332-335 Timestamp: 2025-05-12T19:49:35.667Z Learning: In the Blackbox Log Viewer, the `_getFlightSamplesFreq()` function should return a sliced array `samples.slice(0, samplesCount)` rather than the full preallocated array, as code that consumes this function expects the array length to match the count of valid samples.Learnt from: Quick-Flash PR: betaflight/blackbox-log-viewer#0 File: :0-0 Timestamp: 2025-05-13T17:48:20.477Z Learning: When using FFT (Fast Fourier Transform), enforcing power-of-2 input lengths improves computational efficiency and numerical stability but can result in discarding up to almost 50% of the flight data in worst-case scenarios.public/js/webworkers/spectrum-export-worker.js (3)
7-7: No internal CSV parsers depend on “freq”/“value” headersWe ran a global search for occurrences of the old headers and didn’t find any import routines that parse CSV by header name—only export workers reference the new headers “x” and “y.”
• public/js/webworkers/spectrum-export-worker.js – writes “x” + delimiter + “y”
• public/js/webworkers/csv-export-worker.js and related exporters – join columns by delimiterSince there are no internal consumers of the old “freq”/“value” names, this change won’t break anything in the codebase. If you have external import workflows that expect “freq”/“value,” consider noting the header change in your release notes or docs.
8-11: Confirm the loop change is intentionalThe export worker now outputs every FFT bin (index += 1) instead of stepping by 10, resulting in 10× more data points. This increases frequency resolution but also file size and may impact performance.
• File: public/js/webworkers/spectrum-export-worker.js
Lines 8–11:for (let index = 0; index < spectrumDataLength; index++) { const frequency = frequencyStep * index; outText += frequency + columnDelimiter + fftOutput[index] + "\n"; }Searches for “step 10” or “export resolution” turned up no related settings—please verify that emitting every index is desired and that downstream consumers and file size can handle the increased data.
4-4: Full-spectrum export is correct and aligns with current data shapeWe’ve confirmed that
fftData.fftOutputalready contains only the one-sided magnitude array (returned by_normalizeFft), so exporting the full array length is the right move—dividing by 2 here would undercount your bins. The header rename to “x”/“y” also matches theGraphSpectrumPlotpoint properties, and stepping by 1 exports every frequency bin at full resolution (the UI plot likewise iterates every index).Points of interest in public/js/webworkers/spectrum-export-worker.js:
const spectrumDataLength = fftOutput.length;(previously/ 2)const frequencyStep = 0.5 * event.data.blackBoxRate / spectrumDataLength;let outText = "x" + columnDelimiter + "y" + "\n";for (let index = 0; index < spectrumDataLength; index++) { … }Note: if you have downstream CSV-import workflows that expected “freq”/“value” or sampled every 10th bin, you’ll need to update them to handle the new headers and full data.
index.html (1)
2992-3014: Persistedanalyser-legend-*settings are handled correctlyThe explicit selectors in
src/user_settings_dialog.jsmap the three-part input names to theanalyser_legendobject, so no renaming or parser changes are needed:• In
src/user_settings_dialog.js, lines ~309–312 read
—input[name="analyser-legend-top"]→analyser_legend.top
—input[name="analyser-legend-left"]→analyser_legend.left
—input[name="analyser-legend-width"]→analyser_legend.width• In the same file, lines ~663–669 restore those values back into the inputs.
Minor suggestion: add
inputmode="numeric"to the three<input>elements inindex.htmlfor improved mobile UX.src/main.js (3)
1152-1166: Good refactoring to centralize filename generation logic.The removal of the
fileparameter and dynamic filename retrieval fromgraph.getAnalyser().getExportedFileName()improves encapsulation. The null check with early return and warning message provides good error handling for unsupported spectrum types.
1752-1752: Method name change improves consistency.The rename from
clearImportedSpectrumstoremoveImportedSpectrumsprovides better semantic clarity and aligns with the refactored spectrum management system.
2457-2462: Well-implemented keyboard shortcut addition.The Insert key shortcut for adding current spectrum PSD curve into import list follows established patterns. The
hasAnalysercondition check ensures the feature only works when appropriate, and thepreventDefault()call is correctly placed.src/user_settings_dialog.js (3)
234-238: Consistent addition of analyser legend settings.The new
analyser_legendsettings group follows the established pattern of other UI element positioning settings. The default values (left: 88%, top: 7%, width: 10%) provide reasonable positioning for the legend.
309-313: Proper UI-to-settings conversion implementation.The analyser legend settings conversion correctly follows the same pattern as other settings groups, reading from the appropriate input fields in the
.analyser-settingssection and applying percentage formatting.
663-671: Correct settings-to-UI synchronization.The settings-to-UI population for analyser legend properties correctly extracts integer values from the percentage strings and populates the corresponding input fields, maintaining consistency with other settings groups.
src/graph_spectrum.js (4)
354-356: LGTM! Visibility logic properly updated for spectrum comparison.The change correctly enables the spectrum comparison panel for both FREQUENCY and POWER_SPECTRAL_DENSITY spectrum types, aligning with the PR's objective of supporting PSD spectrum comparison.
420-422: Good refactoring to centralize CSV import logic.The delegation to
GraphSpectrumPlot.importCurvesFromCSVproperly centralizes the CSV parsing logic, reducing code duplication. Error handling is maintained in theImportedCurvesclass as seen in the relevant code snippets.
424-426: Consistent naming and delegation pattern.The method correctly delegates to
GraphSpectrumPlot.removeImportedCurves()and the rename from "clear" to "remove" better reflects the action being performed.
441-443: LGTM! Consistent delegation pattern.The method correctly delegates to
GraphSpectrumPlot.addCurrentSpectrumIntoImport()following the established pattern.src/graph_spectrum_plot.js (4)
60-73: Well-structured initialization of imported curves management.The initialization correctly creates two separate
ImportedCurvesinstances for frequency and PSD spectrums with appropriate redraw callbacks. The color array properly matches the maximum import count of 5.
226-226: Excellent unification of frequency scaling calculations.The introduction of
MAXIMAL_PLOTTED_FREQUENCYconstant and its consistent usage throughout the code improves maintainability and reduces the risk of calculation inconsistencies. The formula correctly represents the Nyquist frequency adjusted for zoom level.Also applies to: 314-314, 330-330, 349-349, 400-400, 470-470, 506-506, 777-777
333-334: Correct implementation of PSD curve drawing.The code properly includes imported PSD curves in the Y-axis range calculation and draws them without smoothing, which is appropriate for PSD data. The frequency limit check correctly prevents drawing beyond the visible range.
Also applies to: 369-384
1778-1818: Well-implemented curve management methods.The three new methods correctly:
- Route imports to the appropriate ImportedCurves instance based on spectrum type
- Handle curve removal for both frequency and PSD types
- Generate PSD curve points with correct frequency calculations and duplicate checking
demvlad
force-pushed
the
psd_curves_multi_select
branch
from
2a7b13e to
7104be8
Compare
|
It works, but i want to add the button "Add" for this action and use shorter Buttons captions: "Imp", "Exp", instead of long Import, Export. |
Co-authored-by: Mark Haslinghuis <[email protected]>
demvlad
dismissed stale reviews from haslinghuis and coderabbitai[bot]
via
bade57d
demvlad
dismissed stale reviews from haslinghuis and coderabbitai[bot]
via
0025b94
|
Added "Add" button, the Insert key works too. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
index.html (3)
480-482: Add Bootstrap button classes for visual & functional consistencyAll control buttons elsewhere in the toolbar carry the
btn&btn-*classes to inherit sizing, spacing and theming.
Without them these three new buttons render unstyled and may miss hover/active states or custom themes.-<button id="btn-spectrum-add" type="button" title="Add spectrum for comparison">Add</button> -<button id="btn-spectrum-export" type="button" title="Export spectrum to CSV">Exp</button> +<button id="btn-spectrum-add" class="btn btn-default" type="button" title="Add spectrum for comparison">Add</button> +<button id="btn-spectrum-export" class="btn btn-default" type="button" title="Export spectrum to CSV">Exp</button>Apply the same
class="btn btn-default"to the “Imp” and “Clr” buttons as well.
This keeps the UI cohesive and avoids ad-hoc CSS overrides later.
482-483: Replace inline-JS with a delegated click handlerInline
onclickattributes are hard to lint, test and minify; the codebase already wires events insrc/main.js.
Add anid(e.g.btn-spectrum-import-trigger) to the visible button and register the click listener in JS, forwarding to the hidden file input:<button id="btn-spectrum-import-trigger" class="btn btn-default" type="button" title="Import spectrum from CSV">Imp</button> <input type="file" id="btn-spectrum-import" accept=".csv" style="display:none" multiple />// src/main.js (near other toolbar bindings) $('#btn-spectrum-import-trigger').on('click', () => $('#btn-spectrum-import').click());This keeps markup clean and centralises behaviour.
480-484: Provide explicit accessible names for abbreviated labelsShort captions (“Exp”, “Imp”, “Clr”) save space but are ambiguous to screen-reader users.
Addaria-label(or keep full text inside a visually-hidden span) so assistive tech announces meaningful actions:-<button ... title="Export spectrum to CSV">Exp</button> +<button ... aria-label="Export spectrum to CSV" title="Export spectrum to CSV">Exp</button>Repeat for “Imp”, “Clr”, and “Add”.
Small change, big accessibility win.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
index.html(1 hunks)src/graph_spectrum_plot.js(4 hunks)src/main.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main.js
- src/graph_spectrum_plot.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
Learnt from: nerdCopter
PR: betaflight/blackbox-log-viewer#822
File: .github/workflows/deploy-preview.yml:17-19
Timestamp: 2025-05-14T16:08:27.254Z
Learning: For betaflight/blackbox-log-viewer repository, the team is aware of the security implications of using `pull_request_target` without specifying `persist-credentials: false` in the checkout action, and has chosen to proceed with the current approach. This concern should not be flagged in future reviews.
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.
index.html (2)
undefined
<retrieved_learning>
Learnt from: demvlad
PR: #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.
</retrieved_learning>
<retrieved_learning>
Learnt from: demvlad
PR: betaflight/blackbox-log-viewer#0
File: :0-0
Timestamp: 2025-06-29T18:40:50.980Z
Learning: In betaflight/blackbox-log-viewer, when implementing PSD heatmap settings persistence in src/graph_spectrum.js, the initialization sequence is critical - user settings must be initialized and applied to GraphSpectrumPlot before attaching input event handlers to prevent spurious refresh() calls that can cause spectrum rendering errors when disabled.
</retrieved_learning>
|
Resolved issue: |
|
The Ctrl+Mouse click by curves legend removes existing spectrum from comparison. |
|
@coderabbitai check last commits please. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 287845 @demvlad I've checked the latest commit, but the diff shows a full repository snapshot (likely after a squash or force-push), so incremental changes aren't visible. However, based on your recent comments and the PR objectives, here's what I can confirm:
These changes align perfectly with the requested UX and complete the feature set for single-log PSD spectrum comparison. If you want a line-by-line review of the new removal logic, let me know which file to focus on! Great job—this is a well-executed and user-friendly implementation. 🐰 ✏️ Learnings added
🧠 Learnings used |
@haslinghuis I have nothing else to add. |
@mituritsyn |
|
Tip For best results, initiate chat on the files or code changes. @demvlad You're very welcome! I'm glad the CSS guidance for making the numeric input spin buttons visible was helpful for your PSD heatmap implementation. If you need any assistance with styling improvements in the future, feel free to ask. The numeric input controls can definitely be made more user-friendly with proper CSS styling. Great work on this comprehensive PSD spectrum comparison feature! 🐰 🧠 Learnings used |
1 similar comment
|
Tip For best results, initiate chat on the files or code changes. @demvlad You're very welcome! I'm glad the CSS guidance for making the numeric input spin buttons visible was helpful for your PSD heatmap implementation. If you need any assistance with styling improvements in the future, feel free to ask. The numeric input controls can definitely be made more user-friendly with proper CSS styling. Great work on this comprehensive PSD spectrum comparison feature! 🐰 🧠 Learnings used |
There was a problem hiding this comment.
Pull Request Overview
This PR adds the ability to compare PSD (Power Spectral Density) spectrums from different curves within a single log file without requiring external CSV import/export. Users can now select a curve, press Insert or "Add" to add its spectrum to a comparison list, then select other curves to compare their spectrums side-by-side.
- Enhanced spectrum comparison functionality with single log file support
- Added Ctrl key support for spectrum analyser control and curve management
- Improved UI with shortened button labels and dynamic "Add" button visibility
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.js | Enhanced legend selection to support Ctrl key for spectrum analyser control |
| src/grapher.js | Added multi-spectrum detection and spectrum preparation for comparison |
| src/graph_spectrum_plot.js | Core spectrum comparison logic with curve color management and drawing improvements |
| src/graph_spectrum.js | Spectrum analyser coordination and UI control for comparison mode |
| src/graph_legend.js | Added Ctrl key event passing to selection handlers |
| src/graph_imported_curves.js | Refactored curve management with proper encapsulation and range calculation |
| index.html | Shortened spectrum comparison button labels |
Comments suppressed due to low confidence (3)
src/graph_spectrum_plot.js:272
- Variable name 'curvesPoints' should be 'curvePoints' (singular) since it represents points for a single curve, not multiple curves.
const curvesPoints = this._importedSpectrums.getCurve(spectrumNum).points;
src/graph_spectrum_plot.js:384
- Variable name 'curvesPoints' should be 'curvePoints' (singular) since it represents points for a single curve, not multiple curves.
const curvesPoints = this._importedPSD.getCurve(spectrumNum).points;
src/graph_spectrum_plot.js:175
- Use strict equality (===) instead of loose equality (==) when comparing with null for more predictable behavior.
0,
|
|
Preview URL: https://f3157a35.betaflight-blackbox.pages.dev |
|
The last notice: |
|
@demvlad is this ready or not 🤣 |
The current implementation is working. |
This is solution for quick comparison of PSD spectrums for one log file data, without using of Export/Import data to external file.
How to do this:
The Export/Import buttons uses for multiple log files data comparison.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements