Using of number input field for PSD segments length #867
Conversation
WalkthroughAdds a PSD segment-length numeric control and label, reorders PSD inputs/labels, updates CSS for new elements, wires the control into spectrum UI (events, visibility, positioning), and extends PSD calculation with configurable points-per-segment and a maximalSegmentsLength result field. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Spectrum UI
participant Handler as UI Handler
participant Calc as GraphSpectrumCalc
participant PSD as PSD loader
User->>UI: change analyserSegmentLengthPowerAt2
UI->>Handler: input change event
Handler->>Calc: setPointsPerSegmentPSD(n)
Handler->>Calc: request PSD reload
Calc->>PSD: dataLoadPSD(pointsPerSegment=n)
PSD-->>Calc: psdData (includes maximalSegmentsLength)
Calc-->>Handler: updated PSD data
Handler->>UI: update plot and controls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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
🧹 Nitpick comments (4)
src/css/main.css (1)
690-704: UI positioning is brittle; consider local flex container for analyser controls.Absolute pixel offsets for the new input/labels will drift across sizes/themes. A tiny flex/grid wrapper around the analyser controls would simplify alignment and improve responsiveness. No functional change needed now.
Also applies to: 751-756, 758-765, 713-719, 728-734, 743-749
src/graph_spectrum.js (2)
163-166: Clamp the control value if current exponent exceeds new max.When max exponent decreases (e.g., shorter selection), ensure the current value is within range to avoid inconsistent UI/state.
- analyserSegmentLengthPowerAt2.prop("max", Math.ceil(Math.log2(fftData.maximalSegmentsLength))); + const maxExp = Math.ceil(Math.log2(fftData.maximalSegmentsLength)); + analyserSegmentLengthPowerAt2.prop("max", maxExp); + const curExp = parseInt(analyserSegmentLengthPowerAt2.val(), 10); + if (Number.isFinite(curExp) && curExp > maxExp) { + analyserSegmentLengthPowerAt2.val(maxExp).trigger("input"); + }
332-351: Remove unused variable and persist the setting (optional).
- segmentLengthPower2 is unused. Remove it.
- Consider saving the chosen exponent in userSettings (like Min/Max PSD) so it survives reloads. Optional, but consistent with other controls.
- let segmentLengthPower2 = DEFAULT_PSD_SEGMENT_LENGTH_POWER; - GraphSpectrumCalc.setPointsPerSegmentPSD(2 ** DEFAULT_PSD_SEGMENT_LENGTH_POWER); + GraphSpectrumCalc.setPointsPerSegmentPSD(2 ** DEFAULT_PSD_SEGMENT_LENGTH_POWER);index.html (1)
508-517: Associate label and input for accessibility.Tie the label to the input so screen readers announce it and clicking the label focuses the input.
- <input id="analyserSegmentLengthPowerAt2" class="onlyFullScreen" type="number" name="analyserSegmentLengthPowerAt2" value="9" min="6" max="20" step="1" - /> - <label id="analyserSegmentLengthPowerAt2Label" name="analyserSegmentLengthPowerAt2Label" class="onlyFullScreen" > + <input id="analyserSegmentLengthPowerAt2" + class="onlyFullScreen" + type="number" + name="analyserSegmentLengthPowerAt2" + value="9" min="6" max="20" step="1" + aria-labelledby="analyserSegmentLengthPowerAt2Label"/> + <label id="analyserSegmentLengthPowerAt2Label" + for="analyserSegmentLengthPowerAt2" + name="analyserSegmentLengthPowerAt2Label" + class="onlyFullScreen"> Segment length <br>power at 2: </label>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
index.html(1 hunks)src/css/main.css(2 hunks)src/graph_spectrum.js(7 hunks)src/graph_spectrum_calc.js(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-26T16:18:25.863Z
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.
Applied to files:
src/graph_spectrum.jssrc/graph_spectrum_calc.js
📚 Learning: 2025-06-29T18:40:50.980Z
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.
Applied to files:
src/graph_spectrum.jssrc/graph_spectrum_calc.js
🧬 Code graph analysis (2)
src/graph_spectrum.js (2)
src/graph_spectrum_calc.js (7)
fftData(111-111)fftData(218-226)fftData(314-314)fftData(321-321)fftData(622-629)GraphSpectrumCalc(24-39)GraphSpectrumCalc(24-39)src/graph_spectrum_plot.js (2)
SPECTRUM_TYPE(18-26)SPECTRUM_TYPE(18-26)
src/graph_spectrum_calc.js (1)
src/graph_spectrum_plot.js (2)
pointsCount(273-273)pointsCount(354-354)
🔇 Additional comments (1)
src/graph_spectrum.js (1)
384-393: Visibility toggles look correct.New analyserSegmentLengthPowerAt2 control and its label are shown only for POWER_SPECTRAL_DENSITY; matches intent.
Also applies to: 418-421
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/graph_spectrum.js (1)
351-357: Do not block all key presses on the number input (breaks keyboard nav).As noted in previous reviews, calling preventDefault on every keydown disables arrows, digits, Tab, and other keys, creating an accessibility blocker. Remove this handler to allow native number input keyboard interaction.
- analyserSegmentLengthPowerAt2 - .on( - "keydown", - function (e) { - e.preventDefault(); - }, - );
🧹 Nitpick comments (2)
src/graph_spectrum.js (2)
165-165: Consider defensive validation for maximalSegmentsLength.While the code should work correctly when PSD data is properly loaded, adding a defensive check would prevent potential issues if the data structure is incomplete.
- analyserSegmentLengthPowerAt2.prop("max", Math.ceil(Math.log2(fftData.maximalSegmentsLength))); + if (fftData.maximalSegmentsLength) { + analyserSegmentLengthPowerAt2.prop("max", Math.ceil(Math.log2(fftData.maximalSegmentsLength))); + }
332-349: Add debounce and consider persisting the segment length setting.The input handler triggers potentially expensive operations (data reload, plot update) without debouncing, which could cause performance issues if the user rapidly changes values. Additionally, unlike other PSD settings (psdHeatmapMin/Max), this setting isn't persisted to userSettings, which means it won't be remembered across sessions.
+ if (userSettings.psdSegmentLengthPower == undefined) { + userSettings.psdSegmentLengthPower = DEFAULT_PSD_SEGMENT_LENGTH_POWER; + } + GraphSpectrumCalc.setPointsPerSegmentPSD(2 ** userSettings.psdSegmentLengthPower); + - GraphSpectrumCalc.setPointsPerSegmentPSD(2 ** DEFAULT_PSD_SEGMENT_LENGTH_POWER); analyserSegmentLengthPowerAt2 .on( "input", - function () { + debounce(100, function () { - // Recalculate PSD with updated samples per segment count - GraphSpectrumCalc.setPointsPerSegmentPSD(2 ** Number.parseInt($(this).val())); + const power = Number.parseInt($(this).val()); + GraphSpectrumCalc.setPointsPerSegmentPSD(2 ** power); + saveOneUserSetting("psdSegmentLengthPower", power); dataLoad(); GraphSpectrumPlot.setData(fftData, userSettings.spectrumType); that.refresh(); - }, + }), ) .dblclick(function (e) { if (e.ctrlKey) { - $(this).val(DEFAULT_PSD_SEGMENT_LENGTH_POWER).trigger("input"); + $(this).val(userSettings.psdSegmentLengthPower).trigger("input"); } }) - .val(DEFAULT_PSD_SEGMENT_LENGTH_POWER); + .val(userSettings.psdSegmentLengthPower);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/graph_spectrum.js(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-26T16:18:25.863Z
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.
Applied to files:
src/graph_spectrum.js
📚 Learning: 2025-06-29T18:40:50.980Z
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.
Applied to files:
src/graph_spectrum.js
🧬 Code graph analysis (1)
src/graph_spectrum.js (2)
src/graph_spectrum_calc.js (7)
fftData(111-111)fftData(218-226)fftData(314-314)fftData(321-321)fftData(622-629)GraphSpectrumCalc(24-39)GraphSpectrumCalc(24-39)src/graph_spectrum_plot.js (2)
SPECTRUM_TYPE(18-26)SPECTRUM_TYPE(18-26)
🔇 Additional comments (3)
src/graph_spectrum.js (3)
20-21: LGTM!The default PSD segment length power (2^9 = 512 points) is a reasonable default for balancing frequency resolution and curve smoothness.
129-134: LGTM!The positioning logic for the new control and label follows the established pattern and integrates well with the existing UI layout.
382-420: LGTM!The visibility logic correctly shows the segment length control and its label only for PSD curve mode, and appropriately includes PSD curve in the Y-zoom visibility conditions.
|
|
Preview URL: https://pr867.betaflight-blackbox.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/graph_spectrum.js (1)
332-349: Good initialization order, but missing debounce for expensive operation.The initialization sequence is correct (setting the value before attaching handlers, per learnings), and the ctrl+double-click reset is a nice touch. However, the input handler lacks debouncing that all other similar controls use:
analyserZoomXElem.on("input", debounce(100, function() { ... })) analyserMinPSD.on("input", debounce(100, function() { ... })) // etc.PSD recalculation (calling
dataLoad,setData,refresh) is expensive. Without debouncing, rapid input changes (arrow keys, typing) trigger multiple expensive recalculations, potentially causing UI lag.Apply this diff to add debouncing consistent with other controls:
GraphSpectrumCalc.setPointsPerSegmentPSD(2 ** DEFAULT_PSD_SEGMENT_LENGTH_POWER); analyserSegmentLengthPowerAt2 .on( "input", - function () { + debounce(100, function () { // Recalculate PSD with updated samples per segment count GraphSpectrumCalc.setPointsPerSegmentPSD(2 ** Number.parseInt($(this).val())); dataLoad(); GraphSpectrumPlot.setData(fftData, userSettings.spectrumType); that.refresh(); - }, + }), )Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/graph_spectrum.js(7 hunks)src/graph_spectrum_calc.js(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-26T16:18:25.863Z
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.
Applied to files:
src/graph_spectrum_calc.jssrc/graph_spectrum.js
📚 Learning: 2025-06-29T18:40:50.980Z
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.
Applied to files:
src/graph_spectrum_calc.jssrc/graph_spectrum.js
🧬 Code graph analysis (2)
src/graph_spectrum_calc.js (1)
src/graph_spectrum_plot.js (2)
pointsCount(273-273)pointsCount(354-354)
src/graph_spectrum.js (2)
src/graph_spectrum_calc.js (7)
fftData(111-111)fftData(217-225)fftData(313-313)fftData(320-320)fftData(621-628)GraphSpectrumCalc(24-39)GraphSpectrumCalc(24-39)src/graph_spectrum_plot.js (2)
SPECTRUM_TYPE(18-26)SPECTRUM_TYPE(18-26)
🔇 Additional comments (7)
src/graph_spectrum_calc.js (3)
38-38: LGTM! Safe default prevents zero-step loops.The default value of 64 (2^6) is a safe minimum that prevents zero-step loops in
_fft_segmented, addressing the concern from previous reviews.
116-118: LGTM! Simple setter is appropriate per user's confirmation.Per your earlier feedback, the UI controls the input and provides only valid power-of-2 values, so the simple setter implementation without defensive validation is appropriate.
120-144: Excellent refactor! All past review concerns addressed.The PSD calculation now correctly:
- Uses actual sample count (
flightSamples.count) instead of padded length- Ensures
overlapCountis an integer withMath.floor- Slices samples to avoid zero-padding bias
- Simplifies logic by removing the unreachable branch (per your confirmation)
This matches the solution discussed in previous reviews.
src/graph_spectrum.js (4)
21-21: LGTM! Reasonable default for PSD segment length.The default power of 9 (yielding 512 points per segment) provides a good balance between frequency resolution and curve smoothing for typical flight log analysis.
129-134: LGTM! CSS positioning consistent with other controls.The positioning logic for the new segment length control and its label follows the same pattern as other spectrum controls.
163-166: LGTM! Dynamic max bound prevents invalid segment lengths.The code correctly updates the input's max attribute based on the actual data length, preventing users from selecting segment lengths that exceed available samples. The
Math.ceil(Math.log2(...))conversion properly translates data length to exponent.
375-413: LGTM! Visibility logic correctly shows/hides PSD controls.The visibility toggles appropriately:
- Hide Y zoom for PSD curve (where zoom is not applicable)
- Show segment length control only for PSD curve (where it's relevant)
- Show/hide label in sync with the input control
The logic is correct and maintains UI consistency.
2 participants
Using of number field to input PSD segments length setting instead of vertical slider.
The vertical slider was looking like curves zoom, but this settings is not a zoom.
The segments length value is inputed as its power at 2 value (N is exponent 2: 26....2N).
The real length is big power at 2 value and input its exponent 2 is more comfortable.
The less value gives more segments count to compute overage PSD value - it gives the smoother curve and less. frequency resolution. The bigger value gives less segments count - the bigger frequency resolution and rough curve.
Summary by CodeRabbit
New Features
UI/Style Updates
Refactor