Added PSD curves export and import to compare several spectrums at the chart #845
Conversation
|
""" WalkthroughThe changes refactor spectrum import and export logic by introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant FlightLogAnalyser
participant GraphSpectrumPlot
participant ImportedCurves
User->>UI: Select CSV files for import
UI->>FlightLogAnalyser: importSpectrumFromCSV(files)
FlightLogAnalyser->>GraphSpectrumPlot: importCurvesFromCSV(files)
GraphSpectrumPlot->>ImportedCurves: importCurvesFromCSV(files)
ImportedCurves-->>GraphSpectrumPlot: Curves imported, state updated
GraphSpectrumPlot-->>FlightLogAnalyser: Import complete
FlightLogAnalyser-->>UI: Update display
User->>UI: Click export spectrum
UI->>FlightLogAnalyser: getExportedFileName()
FlightLogAnalyser-->>UI: Return filename
UI->>WebWorker: Export data with filename
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
Documentation and Community
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/graph_spectrum_plot.js (1)
276-298: Use the defined constant and fix typos.The
filterPointsCountis defined but not used. Also fix the typo throughout:const filterPointsCount = 100; 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 += curvesPoints[i].y; } middleValue /= filterPointsCount; - canvasCtx.lineTo(curvesPonts[pointNum].x * scaleX, HEIGHT - middleValue * fftScale); + canvasCtx.lineTo(curvesPoints[pointNum].x * scaleX, HEIGHT - middleValue * fftScale);
🧹 Nitpick comments (3)
src/graph_spectrum.js (1)
428-439: Clear filename generation based on spectrum type.Consider adding an explicit
defaultcase to make the intent clearer for unsupported spectrum types:this.getExportedFileName = function() { let fileName = null; switch (userSettings.spectrumType) { case SPECTRUM_TYPE.FREQUENCY: fileName = "bf_spectrum"; break; case SPECTRUM_TYPE.POWER_SPECTRAL_DENSITY: fileName = "bf_psd"; break; + default: + fileName = null; + break; } return fileName; };src/graph_spectrum_plot.js (2)
266-269: Fix typo in variable name.-const curvesPonts = this._importedSpectrums._curvesData[spectrumNum].points; -const pointsCount = curvesPonts.length; +const curvesPoints = this._importedSpectrums._curvesData[spectrumNum].points; +const pointsCount = curvesPoints.length;
435-458: Add missing semicolon.canvasCtx.strokeText(curvesName, legendPosX + padding, Y); } -} +};
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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(8 hunks)src/main.js(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common 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.
public/js/webworkers/spectrum-export-worker.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: 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/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.
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/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_plot.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.
🧬 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-75)
🪛 ESLint
src/graph_imported_curves.js
[error] 12-13: Missing semicolon.
(semi)
[error] 74-75: Missing semicolon.
(semi)
src/graph_spectrum_plot.js
[error] 457-458: Missing semicolon.
(semi)
🪛 GitHub Check: SonarCloud
src/graph_spectrum_plot.js
[notice] 373-373: Unused assignments should be removed
Remove this useless assignment to variable "pointsCount".See more on SonarQube Cloud
[notice] 379-379: Unused assignments should be removed
Remove this useless assignment to variable "filterPointsCount".See more on SonarQube Cloud
🔇 Additional comments (15)
src/graph_spectrum_calc.js (1)
133-134: LGTM: Property name alignment improves consistencyThe renaming of
psdLengthtofftLengthandpsdOutputtofftOutputaligns with the broader refactoring to standardize naming conventions across the spectrum analysis modules. The underlying functionality remains unchanged.src/graph_imported_curves.js (2)
14-65: Well-structured CSV import implementationThe CSV import logic is well-implemented with proper:
- File validation and header checking
- Asynchronous file reading
- Error handling with user-friendly alerts
- Min/max value tracking across all curves
- Import count limiting (maxImportCount = 5)
The CSV format validation ensures compatibility with the export format used elsewhere in the codebase.
67-74: Proper cleanup in removeCurves methodThe method correctly clears all stored data and resets min/max values to their initial extremes, ensuring proper state management.
public/js/webworkers/spectrum-export-worker.js (2)
7-7: Header standardization aligns with import formatChanging the headers from "freq" and "value" to "x" and "y" standardizes the CSV format and ensures compatibility with the new
ImportedCurvesmodule that expects this generic format.
8-8: Export all data points for completenessThe change from stepping by 10 to iterating through all indices (
index < spectrumDataLength) ensures that all frequency-value pairs are exported, providing complete data rather than a sampled subset.src/main.js (3)
152-158: Improved filename handling with proper null checkThe refactoring to obtain the filename from
graph.getAnalyser().getExportedFileName()centralizes the filename logic and provides better error handling. The null check ensures graceful handling of unsupported spectrum types.
1741-1741: Function call updated to match new signatureThe removal of arguments from
exportSpectrumToCsv()aligns with the function's new implementation that obtains the filename internally.
1752-1752: Method name updated to match new APIThe rename from
clearImportedSpectrums()toremoveImportedSpectrums()reflects the updated method naming in the refactored spectrum management system.src/graph_spectrum.js (3)
354-356: Good use of named constants instead of magic numbers.The updated visibility logic correctly shows the spectrum comparison panel for both FREQUENCY and POWER_SPECTRAL_DENSITY types, which aligns well with the PR's objective of supporting PSD curves import/export.
420-422: Excellent refactoring to centralize CSV import logic.Delegating the CSV parsing and validation to
GraphSpectrumPlot.importCurvesFromCSVimproves code organization and reduces duplication.
424-426: Consistent delegation pattern for removing imported curves.The method correctly delegates to
GraphSpectrumPlot.removeImportedCurves(), maintaining consistency with the import functionality.src/graph_spectrum_plot.js (4)
4-4: Well-structured integration of ImportedCurves module.The separation of imported curves into frequency and PSD instances provides clear organization, and the callback pattern ensures proper UI updates when curves change.
Also applies to: 60-73
336-338: Good integration of imported curve ranges.The y-axis range calculation correctly considers both FFT data and imported PSD curves to ensure all data is visible.
459-464: Consistent property naming update.The change from
psdOutputtofftOutputaligns with the overall refactoring and maintains consistency across the codebase.
1773-1793: Well-designed delegation to type-specific ImportedCurves instances.The methods correctly route import and removal operations to the appropriate ImportedCurves instance based on the current spectrum type. This maintains good separation of concerns.
|
Added export and import PSD curves to compare much results. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/graph_spectrum_plot.js (2)
268-269: Fix typo and remove unused variables as flagged by static analysis.Multiple issues need to be addressed:
curvesPontsis a typo and should becurvesPointspointsCountis assigned but never usedfilterPointsCountshould be100(not a variable)Apply this diff to fix the issues:
- const curvesPonts = this._importedSpectrums._curvesData[spectrumNum].points; - const pointsCount = curvesPonts.length; + const curvesPoints = this._importedSpectrums._curvesData[spectrumNum].points; + const pointsCount = curvesPoints.length; const scaleX = 2 * WIDTH / PLOTTED_BLACKBOX_RATE * this._zoomX; canvasCtx.beginPath(); canvasCtx.lineWidth = 1; canvasCtx.strokeStyle = this.curvesColors[spectrumNum]; canvasCtx.moveTo(0, HEIGHT); - const filterPointsCount = 100; + const filterPointsCount = 100; 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 += curvesPoints[i].y; } middleValue /= filterPointsCount; - canvasCtx.lineTo(curvesPonts[pointNum].x * scaleX, HEIGHT - middleValue * fftScale); + canvasCtx.lineTo(curvesPoints[pointNum].x * scaleX, HEIGHT - middleValue * fftScale); } canvasCtx.stroke();Also applies to: 276-276, 293-293, 297-297
370-383: Fix typo and remove unused variables (same issues as frequency graph).The same typo and unused variable issues exist in the PSD drawing section:
Apply this diff to fix the issues:
const spectrumCount = this._importedPSD.curvesCount(); for (let spectrumNum = 0; spectrumNum < spectrumCount; spectrumNum++) { - const curvesPonts = this._importedPSD._curvesData[spectrumNum].points; - const pointsCount = curvesPonts.length; + const curvesPoints = this._importedPSD._curvesData[spectrumNum].points; canvasCtx.beginPath(); canvasCtx.lineWidth = 1; canvasCtx.strokeStyle = this.curvesColors[spectrumNum]; canvasCtx.moveTo(0, HEIGHT); - const filterPointsCount = 100; - for (const point of curvesPonts) { + for (const point of curvesPoints) { canvasCtx.lineTo(point.x * scaleX, HEIGHT - (point.y - minY) * scaleY); } canvasCtx.stroke(); }
🧹 Nitpick comments (1)
src/graph_spectrum_plot.js (1)
266-267: Consider boundary checking for curve colors.The code assumes
curvesColorshas enough colors for all imported curves. Consider adding boundary checking:const spectrumCount = this._importedSpectrums._curvesData.length; for (let spectrumNum = 0; spectrumNum < spectrumCount; spectrumNum++) { const curvesPoints = this._importedSpectrums._curvesData[spectrumNum].points; const pointsCount = curvesPoints.length; const scaleX = 2 * WIDTH / PLOTTED_BLACKBOX_RATE * this._zoomX; canvasCtx.beginPath(); canvasCtx.lineWidth = 1; - canvasCtx.strokeStyle = this.curvesColors[spectrumNum]; + canvasCtx.strokeStyle = this.curvesColors[spectrumNum % this.curvesColors.length];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/graph_imported_curves.js(1 hunks)src/graph_spectrum_plot.js(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/graph_imported_curves.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common 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.
src/graph_spectrum_plot.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.
🔇 Additional comments (8)
src/graph_spectrum_plot.js (8)
4-4: LGTM: Import statement is correctly added.The import of
ImportedCurvesaligns with the refactoring to modularize imported curve management.
60-68: LGTM: New properties are well-structured.The separation of
_importedSpectrumsand_importedPSDprovides clear distinction between frequency and PSD curves. ThecurvesColorsarray provides a consistent color palette for imported curves.
72-73: LGTM: Proper initialization with redraw callbacks.The initialization of
ImportedCurvesinstances with callbacks to trigger redraws ensures the UI stays in sync when curves are modified.
140-144: LGTM: Unified redraw method implementation.The
redrawmethod properly invalidates both caches and triggers a draw, providing a clean way to refresh the display when imported curves change.
336-337: LGTM: Improved y-axis range calculation.The updated logic correctly incorporates both FFT data and imported PSD curves when calculating the y-axis range, ensuring all data is properly visible.
434-456: LGTM: Well-structured legend drawing method.The extracted
_drawLegendmethod provides clean separation of concerns and properly handles legend rendering for both frequency and PSD graphs.
458-463: LGTM: Updated to use consistent data structure.The method now correctly uses
fftOutputinstead ofpsdOutput, aligning with the internal data structure naming changes.
1772-1792: LGTM: Clean delegation pattern for import/export operations.The new methods properly delegate to the appropriate
ImportedCurvesinstance based on the current spectrum type, providing a clean API for the UI layer.
demvlad
changed the title
|
The spectrums export was broken in previouse big PSD PR. It has wrong frequency values. |
|
maybe it makes sense to move towards uploading multiple bbl files at once? or is this an intermediate step towards such functionality? |
|
It would be better to save the file with a name that is the same as the current log. |
|
Is it hard to make plot legends draggable, so you can move them around with just a drag and drop action? or just move them to the top-right corner?
put this setting in advanced menu is also an option |
Yes, i think about using whole logs, but this needs more time. This PR is simple solution. We can compare spectrums for any logs and logs parts. |
Will improve step by step
|
Will try, for standard log name it will work, but it will maybe too long name length to place into legend by custom log file name |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/graph_spectrum_plot.js (1)
267-296: Fix variable name typo.There's a typo in the variable name that needs to be corrected for consistency and readability.
- const curvesPonts = this._importedSpectrums._curvesData[spectrumNum].points; + const curvesPoints = this._importedSpectrums._curvesData[spectrumNum].points;Also update the references:
- middleValue += curvesPonts[i].y; + middleValue += curvesPoints[i].y;- canvasCtx.lineTo(curvesPonts[pointNum].x * scaleX, HEIGHT - middleValue * fftScale); + canvasCtx.lineTo(curvesPoints[pointNum].x * scaleX, HEIGHT - middleValue * fftScale);
🧹 Nitpick comments (1)
src/graph_spectrum_plot.js (1)
1766-1786: Add default case handling for robustness.The new methods provide a clean interface for curve management. Consider adding default cases to handle other spectrum types gracefully.
GraphSpectrumPlot.importCurvesFromCSV = function(files) { switch (this._spectrumType) { case SPECTRUM_TYPE.FREQUENCY: this._importedSpectrums.importCurvesFromCSV(files); break; case SPECTRUM_TYPE.POWER_SPECTRAL_DENSITY: this._importedPSD.importCurvesFromCSV(files); break; + default: + console.warn('Curve import not supported for spectrum type:', this._spectrumType); + break; } }; GraphSpectrumPlot.removeImportedCurves = function() { switch (this._spectrumType) { case SPECTRUM_TYPE.FREQUENCY: this._importedSpectrums.removeCurves(); break; case SPECTRUM_TYPE.POWER_SPECTRAL_DENSITY: this._importedPSD.removeCurves(); break; + default: + console.warn('Curve removal not supported for spectrum type:', this._spectrumType); + break; } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/js/webworkers/spectrum-export-worker.js(1 hunks)src/graph_spectrum.js(2 hunks)src/graph_spectrum_plot.js(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- public/js/webworkers/spectrum-export-worker.js
- src/graph_spectrum.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.
src/graph_spectrum_plot.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.
🧬 Code Graph Analysis (1)
src/graph_spectrum_plot.js (1)
src/graph_imported_curves.js (1)
ImportedCurves(1-75)
🔇 Additional comments (6)
src/graph_spectrum_plot.js (6)
4-4: LGTM! Clean import addition.The import statement correctly brings in the
ImportedCurvesclass needed for the new curve management functionality.
60-68: Good refactoring to separate frequency and PSD curve management.The new properties improve the separation of concerns by having dedicated
ImportedCurvesinstances for frequency spectrums and PSD data. The color array provides good visual distinction for multiple imported curves.
72-73: Proper initialization with reactive callbacks.The initialization correctly sets up the
ImportedCurvesinstances with redraw callbacks, ensuring the graph updates automatically when imported curves change.
140-144: Clean consolidation of refresh logic.The new
redrawmethod provides a clear interface for triggering graph updates and properly invalidates both caches before redrawing.
427-451: Well-implemented legend functionality.The legend drawing method is properly structured with canvas state management, clipping boundaries, and clear visual design. Good use of the established color scheme for curve identification.
454-457: Correct property access update.The update to use
fftOutputinstead ofpsdOutputaligns with the internal data structure changes while maintaining the same calculation logic.
@mituritsyn |
|
|
Co-authored-by: Mark Haslinghuis <[email protected]>
Co-authored-by: Mark Haslinghuis <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/graph_imported_curves.js (1)
37-49: Consider adding validation for parsed numeric valuesThe CSV parsing doesn't validate that parsed values are valid numbers, which could lead to NaN values in the curve data.
const curvesData = stringRows.map( function(row) { const data = row.split(","), x = parseFloat(data[0]), y = parseFloat(data[1]); + if (isNaN(x) || isNaN(y)) { + throw new SyntaxError(`Invalid numeric values in row: ${row}`); + } _that.minX = Math.min(x, _that.minX); _that.maxX = Math.max(x, _that.maxX); _that.minY = Math.min(y, _that.minY); _that.maxY = Math.max(y, _that.maxY); return { x: x, y: y, }; });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/graph_imported_curves.js(1 hunks)
🧰 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.
src/graph_imported_curves.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: haslinghuis
PR: betaflight/blackbox-log-viewer#820
File: src/graph_spectrum_calc.js:557-557
Timestamp: 2025-05-05T19:23:29.970Z
Learning: In JavaScript object literals, include trailing commas even on the last property for better maintainability and cleaner diffs when new properties are added later.
🪛 Biome (1.9.4)
src/graph_imported_curves.js
[error] 18-18: Can't assign importsLeft because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
🪛 ESLint
src/graph_imported_curves.js
[error] 18-18: 'importsLeft' is constant.
(no-const-assign)
🔇 Additional comments (3)
src/graph_imported_curves.js (3)
1-9: LGTM - Well-structured module initializationThe module follows good practices with proper closure handling and clear initialization of tracking variables for min/max values across imported curves.
10-12: LGTM - Simple and correct accessor methodThe curvesCount function properly encapsulates access to the internal curve data array length.
67-74: LGTM - Correct curve removal implementationThe removeCurves function properly clears all data and resets tracking variables, with appropriate callback notification.
|
|
Preview URL: https://bc309513.betaflight-blackbox.pages.dev |
4 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes