Using of number input field for PSD segments length #867
Conversation
WalkthroughAdds a PSD segment-length control and label to the UI, reorders PSD inputs/labels and CSS, wires the control into spectrum UI events/visibility/positioning, and extends PSD calculation with configurable points-per-segment and a returned maximalSegmentsLength. 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(2**power)
Handler->>Calc: request PSD reload
Calc->>PSD: dataLoadPSD(pointsPerSegment)
PSD-->>Calc: psdData {..., maximalSegmentsLength}
Calc-->>Handler: updated PSD data
Handler->>UI: update plot and control visibility/limits
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Potential attention points:
Possibly related PRs
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.
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.
|
@demvlad , check latest nitpick. yes they do become hassle-some. validate if really needed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/graph_spectrum.js (1)
332-349: Logic is correct; consider minor style consistency.The initialization and event handling for the PSD segment length control is implemented correctly. The ctrl+double-click reset follows the established pattern, and the input handler properly recalculates PSD data.
Note: The keydown preventDefault issue mentioned in previous reviews is not present in the current code.
For consistency with line 358, consider using
parseInt($(this).val(), 10)instead ofNumber.parseInt($(this).val()):- GraphSpectrumCalc.setPointsPerSegmentPSD(2 ** Number.parseInt($(this).val())); + GraphSpectrumCalc.setPointsPerSegmentPSD(2 ** parseInt($(this).val(), 10));
📜 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(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 (4)
src/graph_spectrum.js (4)
20-21: LGTM! PSD segment length constant is well-defined.The default exponent of 9 (yielding 2^9 = 512 points per segment) is a reasonable choice and aligns with the PR objective to provide user control over PSD segmentation.
129-134: LGTM! Positioning logic follows established patterns.The positioning of the new control and its label is consistent with the existing PSD controls and properly integrated into the resize function.
410-413: LGTM! Label visibility is properly synchronized.The label visibility toggle matches the control visibility and follows the same pattern as other PSD-related labels.
375-385: The review comment is based on a misinterpretation of the CSS behavior.The
onlyFullScreenExceptionclass uses a CSS negation selector that only affects visibility in fullscreen mode:input:not(.onlyFullScreenException)within thehas-analyser-fullscreencontext. In normal (non-fullscreen) mode,analyserZoomYElemremains fully visible and functional.The element is not hidden from users in regular operation. The event handler at line 251 remains attached regardless of the class, and
analyserZoomYcan be adjusted normally. In fullscreen mode, the control is intentionally excluded from the display rule, likely by design to prioritize the segment length control for PSD visualization. This is not a problematic hidden dependency—it's intentional UI behavior.Likely an incorrect or invalid review comment.
|
|
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)
334-351: Consider setting a minimum value for the segment length control.The input handler correctly initializes the default value and sets the dynamic max (line 166), but I don't see a minimum value being set. Without a min constraint, users could enter very small values (e.g., 0 or 1), resulting in segment lengths of 1 or 2 points, which would be invalid for PSD calculation.
If the HTML doesn't already define a
minattribute, consider adding it here:GraphSpectrumCalc.setPointsPerSegmentPSD(2 ** DEFAULT_PSD_SEGMENT_LENGTH_POWER); analyserSegmentLengthPowerAt2 + .prop("min", 4) // Minimum segment length of 2^4 = 16 points .on( "input",
📜 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 (2)
GraphSpectrumCalc(24-39)GraphSpectrumCalc(24-39)src/graph_spectrum_plot.js (2)
SPECTRUM_TYPE(18-26)SPECTRUM_TYPE(18-26)
🔇 Additional comments (4)
src/graph_spectrum.js (4)
20-21: LGTM - Clean constant definition and element initialization.The default value of 9 (2^9 = 512 points per segment) is reasonable for PSD calculations, and the element initialization follows the established pattern in this file.
Also applies to: 40-40
129-134: LGTM - Positioning follows established pattern.The CSS positioning for the new segment length control and label is consistent with the existing analyser controls.
165-167: LGTM - Edge case validation properly implemented.The validation checking
maximalSegmentsLength > 0before setting the max attribute correctly addresses the edge case concern from the previous review, preventing NaN or invalid max values.
377-387: The zoom parameter todataLoadPSD()is unused; the design is correct but contains dead code.The code behavior is intentional:
analyserZoomYis passed todataLoadPSD()at line 164, but the implementation never uses it. Zoom affects only plot visualization (viasetZoom()), not PSD calculation. Consequently:
- Hiding the zoom control when PSD is selected (line 380-382) is correct—zoom changes don't affect PSD data.
- The handler not triggering a reload on zoom changes (lines 249-256) is correct—the zoom parameter is unused.
Consider removing the unused
analyserZoomYparameter from thedataLoadPSD()function signature insrc/graph_spectrum_calc.jsat line 120, or if it's reserved for future use, add a comment explaining its intent.
3 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