Power spectral density curves settings impromement #844
Conversation
…or PSD segment length values input
…rm with shows logic
|
""" WalkthroughA new numeric input for segment length selection in Power Spectral Density (PSD) analysis was added to the analyser UI. Supporting logic was implemented to enforce power-of-two values, update PSD calculations, and dynamically adjust limits. Internal PSD calculation logic was refactored to use this user-defined segment length. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AnalyserUI
participant GraphSpectrum
participant GraphSpectrumCalc
User->>AnalyserUI: Change segment length input
AnalyserUI->>GraphSpectrum: Validate and update segment length
GraphSpectrum->>GraphSpectrumCalc: setPointsPerSegmentPSD(newLength)
GraphSpectrum->>GraphSpectrumCalc: dataLoadPSD()
GraphSpectrumCalc-->>GraphSpectrum: PSD data (using new segment length)
GraphSpectrum->>AnalyserUI: Refresh PSD plot
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
index.html (1)
496-515: Wire up PSD segment-length persistence and restoreThe segment-length control is correctly clamped to powers-of-two and toggled based on spectrum type, but it isn’t saved or initialized from user settings. Without this, users will lose their chosen segment length on refresh.
• In src/graph_spectrum.js, around line 302–304 (initialization):
– LoaduserSettings.psdSegmentLength(or fallback toDEFAULT_PSD_SEGMENT_LENGTH) before callingGraphSpectrumCalc.setPointsPerSegmentPSD(...)and set the input’s.val(...).• In the
.on("input", …)handler at line 304–318:
– After updatingsegmentLenghtPSD, callsaveOneUserSetting("psdSegmentLength", segmentLenghtPSD).Suggested diff snippet:
--- a/src/graph_spectrum.js +++ b/src/graph_spectrum.js @@ 300,7 +300,15 @@ - let segmentLenghtPSD = DEFAULT_PSD_SEGMENT_LENGTH; - GraphSpectrumCalc.setPointsPerSegmentPSD(segmentLenghtPSD); - analyserSegmentLengthPSD + // Restore saved PSD segment length + let segmentLenghtPSD = + userSettings.psdSegmentLength ?? DEFAULT_PSD_SEGMENT_LENGTH; + analyserSegmentLengthPSD.val(segmentLenghtPSD); + GraphSpectrumCalc.setPointsPerSegmentPSD(segmentLenghtPSD); + saveOneUserSetting("psdSegmentLength", segmentLenghtPSD); + analyserSegmentLengthPSD @@ 307,6 +315,7 @@ $(this).val(segmentLenghtPSD); // Recalculate PSD with updated samples per segment count GraphSpectrumCalc.setPointsPerSegmentPSD(segmentLenghtPSD); + saveOneUserSetting("psdSegmentLength", segmentLenghtPSD); dataLoad(); GraphSpectrumPlot.setData(fftData, userSettings.spectrumType); that.refresh();This ensures the control’s value is persisted and restored.
🧹 Nitpick comments (2)
index.html (2)
496-497: Associate the new input with its label for accessibilityThe new
<input>lacks an explicit association with its descriptive label (<label id="analyserSegmentLengthPSDLabel">). Adding afor="analyserSegmentLengthPSD"attribute on the label (and keeping the input’sid) will let screen–reader and form–navigation tools recognise the relationship.-<label id="analyserSegmentLengthPSDLabel" name="analyserSegmentLengthPSDLabel" class="onlyFullScreen" > +<label for="analyserSegmentLengthPSD" + id="analyserSegmentLengthPSDLabel" + name="analyserSegmentLengthPSDLabel" + class="onlyFullScreen">
513-515: Minor wording: “Segment length” (singular) reads betterUI copy elsewhere uses singular nouns (“Limit dBm”, “Max dBm”). Consider changing the caption to “Segment length” for consistency and readability.
- Segments length + Segment length
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
index.html(2 hunks)src/css/main.css(2 hunks)src/graph_spectrum.js(8 hunks)src/graph_spectrum_calc.js(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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.
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: 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.
index.html (3)
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>
<retrieved_learning>
Learnt from: demvlad
PR: #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.
</retrieved_learning>
src/css/main.css (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#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: 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 (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: 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.
src/graph_spectrum_calc.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: 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: 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: 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 (12)
src/css/main.css (2)
693-695: LGTM: Consistent spin button styling applied.The CSS rule correctly extends the existing webkit spin button appearance overrides to include the new segment length input, maintaining visual consistency with other PSD inputs.
749-763: LGTM: Proper positioning and styling for new input controls.The CSS positioning and styling for the segment length input and label follow the established pattern of other analyser controls, ensuring consistent appearance and layout.
src/graph_spectrum_calc.js (3)
38-38: LGTM: Appropriate property addition.The new private property
_pointsPerSegmentPSDproperly stores the user-defined segment length for PSD calculations.
116-118: LGTM: Clean setter implementation.The setter method provides a clean interface for updating the PSD segment length from the UI layer.
122-129: Excellent refactoring of PSD calculation logic.The changes successfully decouple PSD segment length from zoom controls, allowing explicit user control. The logic appropriately handles edge cases where the requested segment length exceeds available samples, and the returned
maximalSegmentsLengthenables proper UI validation.Note: The overlap calculation has changed from 50% to 75% (
pointsPerSegment * 3 / 4), which will provide better frequency resolution at the cost of some computational overhead.Also applies to: 142-142
src/graph_spectrum.js (7)
20-21: LGTM: Appropriate default value.The default segment length of 512 is a good power-of-2 value that balances frequency resolution with computational efficiency.
39-39: LGTM: Consistent DOM element reference.The new element reference follows the established naming pattern for other analyser controls.
122-127: LGTM: Proper positioning integration.The positioning logic for the new segment length input and label is properly integrated with the existing resize functionality.
158-158: Excellent dynamic validation integration.Setting the max property based on
maximalSegmentsLengthfrom the FFT data ensures the UI prevents invalid segment length values that exceed available data.
330-336: LGTM: Effective input restriction.The keydown event handler properly prevents arbitrary input, ensuring only power-of-2 values can be set through the UI interaction logic.
362-372: Excellent UI state management.The spectrum type change handling properly shows/hides the segment length control only for the POWER_SPECTRAL_DENSITY type, maintaining clean UI organization and following the established pattern for other spectrum-specific controls.
Also applies to: 397-400
253-253: LGTM: Simplified input handling.Removing the conditional check before setting the low-level PSD value simplifies the logic appropriately.
|
@mituritsyn, please, could you have look at this improvement |
|
functionally it is good. Small visual notice - the signature looks a bit out of place, perhaps it would be bettre to slightly adjust the alignment.
It is appropriately aligned on the frequency plot. |
|
|
Preview URL: https://24aee4a7.betaflight-blackbox.pages.dev |
This was common issue for the all filters at the PSD curve chart only. Resolved. |
The vertical slider is changed to numerical input fields.
The older vertical slider had different function by compare with other spectrums, what was unlogical.
This settings shows points per segment for PSD computing by using Welch method.
This value is power at 2. The bigger segment length gives bigger spectrum frequency resolution, but the less segments length gives smoother result. The maximal value is limited by data samples count.
The new number input field:
The olde vertical slider:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements