revert samples #829
revert samples #829
Conversation
return {
samples : samples,
count : samplesCount,
};
WalkthroughThe update modifies the internal logic of the Changes
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
|
|
@blckmn : |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| return { | ||
| samples : samples.slice(0, samplesCount), | ||
| samples : samples, | ||
| count : samplesCount, | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Properly restores the expected return structure.
The change correctly restores the original behavior by returning the entire samples array without slicing it, while still including the samplesCount as count. This fixes the breakage mentioned in PR #819 where consumers of this function expected the full unsliced array.
🏁 Script executed:
#!/bin/bash
# Let's check how the `samples` array is used throughout the codebase
# Check if any consumers rely on the array length matching the count
rg -A 3 "flightSamples\.samples\.length" --type jsLength of output: 985
🏁 Script executed:
#!/bin/bash
# Display lines 300 to 370 of src/graph_spectrum_calc.js for context
sed -n '300,370p' src/graph_spectrum_calc.js || echo "File not found"Length of output: 2523
Update downstream consumers to use count instead of the array’s capacity
Restoring the full samples array alongside count is correct, but all of the following consumers still use flightSamples.samples.length (the pre-allocated buffer size) rather than flightSamples.count, which will pull in uninitialized zeros past the real data. Please update these sites to use count:
- src/graph_spectrum_calc.js
– FFT normalization:// change this._normalizeFft(fftOutput, flightSamples.samples.length) // to this._normalizeFft(fftOutput, flightSamples.count)
- src/graph_spectrum_calc.js
– Points-per-segment cap:// change pointsPerSegment = Math.min(pointsPerSegment, flightSamples.samples.length); // to pointsPerSegment = Math.min(pointsPerSegment, flightSamples.count);
- src/graph_spectrum_calc.js
– Power spectral density call:// change this._psd(flightSamples.samples, pointsPerSegment, overlapCount); // to this._psd(flightSamples.samples.subarray(0, flightSamples.count), pointsPerSegment, overlapCount);
- src/graph_spectrum_calc.js
– FFT-window loop boundary:// change for (let i = 0; i + fftLen < flightSamples.samples.length; i += window) { … } // to for (let i = 0; i + fftLen < flightSamples.count; i += window) { … }
There was a problem hiding this comment.
@coderabbitai , How can we retain samples : samples.slice(0, samplesCount), and fix any surrounding code that may not have accounted for such?
There was a problem hiding this comment.
🧩 Analysis chain
@nerdCopter, yes, we can definitely retain the slicing operation samples.slice(0, samplesCount) and fix the surrounding code. This is actually a cleaner approach as it ensures that only valid data is passed around.
Here are the changes you would need to make to keep the slicing:
-
Keep the original slicing in
_getFlightSamplesFreq:return { samples : samples.slice(0, samplesCount), count : samplesCount, };
-
No other changes are needed! When you slice the array,
flightSamples.samples.lengthwill correctly be equal toflightSamples.count, which is the desired behavior.
This approach has several advantages:
- Prevents bugs from accidentally processing undefined values past the valid data
- Makes the code self-documenting (array length equals valid sample count)
- Reduces memory usage slightly by not keeping the entire pre-allocated array
Since your PR is reverting a breaking change, keeping the slicing behavior while ensuring the correct structure (including both samples and count properties) should resolve the issue without introducing new problems.
🏁 Script executed:
#!/bin/bash
# Search for all instances where flightSamples.samples.length is used
rg "flightSamples\.samples\.length" src/ --type js
# Also search for places where .samples is used that might rely on the full length
rg "\.samples\b" src/graph_spectrum_calc.js | grep -v "slice" | head -20
Length of output: 982
@nerdCopter Yes, retaining samples : samples.slice(0, samplesCount) is definitely the cleaner approach! After analyzing the code, this appears to be the originally intended pattern.
Looking at the search results, I see code that uses flightSamples.samples.length in three places:
- FFT normalization:
this._normalizeFft(fftOutput, flightSamples.samples.length) - Points-per-segment calculation:
pointsPerSegment = Math.min(pointsPerSegment, flightSamples.samples.length) - A loop condition:
for (let fftChunkIndex = 0; ... < flightSamples.samples.length; ...)
When you use slice, samples.length will equal count, which is exactly what these functions expect. Without the slice, these functions would process the entire preallocated array including unused elements.
I also noticed that _getFlightSamplesFreqVsX() already follows this pattern:
return {
samples: samples.slice(0, samplesCount),
vsValues: slicedVsValues,
count: samplesCount,
// ...
};No other code changes should be needed - just restore the .slice(0, samplesCount) in the return statement, and everything should work correctly.
🧬 Code Graph Analysis Results
Relevant snippets from src/graph_spectrum_calc.js (lines relevant to samples : samples.slice(0, samplesCount), and surrounding code):
GraphSpectrumCalc._getFlightSamplesFreq = function(scaled = true) {
const allChunks = this._getFlightChunks();
const samples = new Float64Array(MAX_ANALYSER_LENGTH / (1000 * 1000) * this._blackBoxRate);
// Loop through all the samples in the chunks and assign them to a sample array ready to pass to the FFT.
let samplesCount = 0;
for (const chunk of allChunks) {
for (const frame of chunk.frames) {
if (scaled) {
samples[samplesCount] = this._dataBuffer.curve.lookupRaw(frame[this._dataBuffer.fieldIndex]);
} else {
samples[samplesCount] = frame[this._dataBuffer.fieldIndex];
}
samplesCount++;
}
}
return {
samples : samples,
count : samplesCount,
};
};- This function collects flight data samples into a preallocated
Float64Arraysized for the maximum analyser length and black box rate. - It tracks the actual number of samples collected in
samplesCount. - It returns the full
samplesarray (which may be larger thansamplesCount) and the count. - The user comment references
samples : samples.slice(0, samplesCount),which is a common pattern to return only the valid portion of the array. - Here, the current code returns the full
samplesarray without slicing, which may cause consumers to process trailing zero or uninitialized data beyondsamplesCount. - Fixing this would involve returning
samples.slice(0, samplesCount)instead of the full array, and ensuring all callers handle the sliced array accordingly.
GraphSpectrumCalc._getFlightSamplesFreqVsX = function(vsFieldNames, minValue = Infinity, maxValue = -Infinity) {
const allChunks = this._getFlightChunks();
const vsIndexes = this._getVsIndexes(vsFieldNames);
const samples = new Float64Array(MAX_ANALYSER_LENGTH / (1000 * 1000) * this._blackBoxRate);
const vsValues = new Array(vsIndexes.length).fill(null).map(() => new Float64Array(MAX_ANALYSER_LENGTH / (1000 * 1000) * this._blackBoxRate));
let samplesCount = 0;
for (const chunk of allChunks) {
for (let frameIndex = 0; frameIndex < chunk.frames.length; frameIndex++) {
samples[samplesCount] = (this._dataBuffer.curve.lookupRaw(chunk.frames[frameIndex][this._dataBuffer.fieldIndex]));
for (let i = 0; i < vsIndexes.length; i++) {
let vsFieldIx = vsIndexes[i];
let value = chunk.frames[frameIndex][vsFieldIx];
if (vsFieldNames == FIELD_RPM_NAMES) {
const maxRPM = MAX_RPM_HZ_VALUE * this._motorPoles / 3.333;
if (value > maxRPM) {
value = maxRPM;
}
else if (value < 0) {
value = 0;
}
}
vsValues[i][samplesCount] = value;
}
samplesCount++;
}
}
// ... code to calculate minValue and maxValue omitted for brevity ...
let slicedVsValues = [];
for (const vsValueArray of vsValues) {
slicedVsValues.push(vsValueArray.slice(0, samplesCount));
}
return {
samples : samples.slice(0, samplesCount),
vsValues : slicedVsValues,
count : samplesCount,
minValue : minValue,
maxValue : maxValue,
};
};- This function similarly preallocates large arrays for samples and vsValues.
- It tracks
samplesCountand returns slices of the arrays up tosamplesCount. - This is the pattern the user comment suggests to retain for
_getFlightSamplesFreqas well. - Surrounding code that consumes this function expects
samplesto be sliced to the actual count. - This function returns
samples: samples.slice(0, samplesCount)andvsValuessliced similarly.
GraphSpectrumCalc.dataLoadFrequency = function() {
const flightSamples = this._getFlightSamplesFreq();
if (userSettings.analyserHanning) {
this._hanningWindow(flightSamples.samples, flightSamples.count);
}
//calculate fft
const fftOutput = this._fft(flightSamples.samples);
// Normalize the result
const fftData = this._normalizeFft(fftOutput, flightSamples.samples.length);
return fftData;
};- This function calls
_getFlightSamplesFreq()and usesflightSamples.samplesdirectly. - If
_getFlightSamplesFreq()returns the full preallocated array without slicing,flightSamples.samples.lengthwill be the max length, not the actual count. - This can cause FFT and normalization to process trailing zero data.
- If
_getFlightSamplesFreq()is changed to returnsamples.slice(0, samplesCount), this function will correctly process only valid data. - Note:
_hanningWindowis called withflightSamples.countas size, so it applies only to valid samples.
GraphSpectrumCalc.dataLoadPSD = function(analyserZoomY) {
const flightSamples = this._getFlightSamplesFreq(false);
let pointsPerSegment = 512;
const multipiler = Math.floor(1 / analyserZoomY); // 0. ... 10
if (multipiler == 0) {
pointsPerSegment = 256;
} else if (multipiler > 1) {
pointsPerSegment *= 2 ** Math.floor(multipiler / 2);
}
pointsPerSegment = Math.min(pointsPerSegment, flightSamples.samples.length);
const overlapCount = Math.floor(pointsPerSegment / 2);
const psd = this._psd(flightSamples.samples, pointsPerSegment, overlapCount);
// ... returns psd data ...
};- Similar to
dataLoadFrequency, this function uses_getFlightSamplesFreq(false)and then usesflightSamples.samples.length. - If
_getFlightSamplesFreqreturns the full preallocated array,samples.lengthis max length, not actual count. - Changing
_getFlightSamplesFreqto return sliced array fixes this. - The
pointsPerSegmentis capped byflightSamples.samples.length, so if length is too large, it may cause issues.
Summary of relevant code context:
- The function
_getFlightSamplesFreqcurrently returns the full preallocatedsamplesarray and a separatecount. - The user comment suggests retaining
samples : samples.slice(0, samplesCount),which means returning only the valid portion of the samples array. - Other functions like
_getFlightSamplesFreqVsXalready use this pattern. - Consumers of
_getFlightSamplesFreq(e.g.,dataLoadFrequency,dataLoadPSD) use.samples.lengthto determine data length, so returning the sliced array aligns with their expectations. - Fixing
_getFlightSamplesFreqto returnsamples: samples.slice(0, samplesCount)and updating all callers to expect this sliced array will ensure consistent and correct processing of sample data.
No other files or snippets from the provided possibly relevant code snippets appear directly related to the handling or slicing of samples arrays in graph_spectrum_calc.js.
✏️ Learnings added
Learnt from: nerdCopter
PR: betaflight/blackbox-log-viewer#829
File: src/graph_spectrum_calc.js:332-335
Timestamp: 2025-05-12T19:49:35.638Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
The all looks good, but fft call hangs...
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
if you find solution, make PR and we close this one. Google AI failing me as well.
Google AI said this, but AI has lead me in wrong directions before, so take with grain-of-salt.
And so far, AI prompting cannot properly craft a proper fix without introducing further bugs.
There was a problem hiding this comment.
maybe quickest to revert #819 , but i''m going offline until tomorrow.
There was a problem hiding this comment.
- These analyzes aligns with having enough resources wouldn't trigger the issue.
#819only changed 3 lines (rest was refactoring)- Agree we should remove slice operations as
sampleCountwould handle it.
There was a problem hiding this comment.
Perhaps, i have some solution. Will push it soon.
|
No, this is realy some strange bug. The all looks as good, but fft.simple hangs. GraphSpectrumCalc._fft = function(samples, type) { if (!type) { const fftLength = samples.length; fft.simple(fftOutput, samples, type); return fftOutput; |
|
It looks interest. |
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20002 @demvlad I've examined the FFT input size in the code. Here are my findings:
const samples = new Float64Array(MAX_ANALYSER_LENGTH / (1000 * 1000) * this._blackBoxRate);
I'd recommend modifying the code to:
This explains why sometimes it runs fast and other times it hangs - the FFT algorithm's performance is highly dependent on input size characteristics. |
|
I confused, ...a small bit. lol:) |
|
@haslinghuis @nerdCopter |
|
This deep dive needs testing. I've reverted both PR. |
|
thank you @demvlad ! i feel the copying the data as per #829 (comment) may still need attention, but i'm unsure how to resolve. and maybe padding to 2^N could be rather simple, i don't know. I've closed this PR and approved your revert. |
|
The power2 version works fast, but it has much changes, it needs a small research, test and compare with source version. But 600000 points is too big for screen charts, i think. By other hand, the power2 maybe not enough for the small time interval. Need to experiment. |
3 participants
graph_spectrum_calc.js:fixes breakage cause by #819
this PR based on a branch. #828 was master by mistake.
Summary by CodeRabbit