Skip to content

revert samples #829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

revert samples #829

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/graph_spectrum_calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ GraphSpectrumCalc._getFlightSamplesFreq = function(scaled = true) {
}

return {
samples : samples.slice(0, samplesCount),
samples : samples,
count : samplesCount,
};
Comment on lines 332 to 335
Copy link

@coderabbitai coderabbitai bot May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 js

Length 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) { … }

Copy link
Member Author

@nerdCopter nerdCopter May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai , How can we retain samples : samples.slice(0, samplesCount), and fix any surrounding code that may not have accounted for such?

Copy link

@coderabbitai coderabbitai bot May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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:

  1. Keep the original slicing in _getFlightSamplesFreq:

    return {
  samples : samples.slice(0, samplesCount),
  count : samplesCount,
};

  2. No other changes are needed! When you slice the array, flightSamples.samples.length will correctly be equal to flightSamples.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:

  1. FFT normalization: this._normalizeFft(fftOutput, flightSamples.samples.length)
  2. Points-per-segment calculation: pointsPerSegment = Math.min(pointsPerSegment, flightSamples.samples.length)
  3. 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 Float64Array sized for the maximum analyser length and black box rate.
  • It tracks the actual number of samples collected in samplesCount.
  • It returns the full samples array (which may be larger than samplesCount) 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 samples array without slicing, which may cause consumers to process trailing zero or uninitialized data beyond samplesCount.
  • 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 samplesCount and returns slices of the arrays up to samplesCount.
  • This is the pattern the user comment suggests to retain for _getFlightSamplesFreq as well.
  • Surrounding code that consumes this function expects samples to be sliced to the actual count.
  • This function returns samples: samples.slice(0, samplesCount) and vsValues sliced 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 uses flightSamples.samples directly.
  • If _getFlightSamplesFreq() returns the full preallocated array without slicing, flightSamples.samples.length will be the max length, not the actual count.
  • This can cause FFT and normalization to process trailing zero data.
  • If _getFlightSamplesFreq() is changed to return samples.slice(0, samplesCount), this function will correctly process only valid data.
  • Note: _hanningWindow is called with flightSamples.count as 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 uses flightSamples.samples.length.
  • If _getFlightSamplesFreq returns the full preallocated array, samples.length is max length, not actual count.
  • Changing _getFlightSamplesFreq to return sliced array fixes this.
  • The pointsPerSegment is capped by flightSamples.samples.length, so if length is too large, it may cause issues.

Summary of relevant code context:

  • The function _getFlightSamplesFreq currently returns the full preallocated samples array and a separate count.
  • The user comment suggests retaining samples : samples.slice(0, samplesCount), which means returning only the valid portion of the samples array.
  • Other functions like _getFlightSamplesFreqVsX already use this pattern.
  • Consumers of _getFlightSamplesFreq (e.g., dataLoadFrequency, dataLoadPSD) use .samples.length to determine data length, so returning the sliced array aligns with their expectations.
  • Fixing _getFlightSamplesFreq to return samples: 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.

Copy link
Contributor

@demvlad demvlad May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The all looks good, but fft call hangs...

Copy link

@coderabbitai coderabbitai bot May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

@nerdCopter nerdCopter May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
image

Copy link
Member Author

@nerdCopter nerdCopter May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe quickest to revert #819 , but i''m going offline until tomorrow.

Copy link
Member

@haslinghuis haslinghuis May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • These analyzes aligns with having enough resources wouldn't trigger the issue.
  • #819 only changed 3 lines (rest was refactoring)
  • Agree we should remove slice operations as sampleCount would handle it.

Copy link
Contributor

@demvlad demvlad May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, i have some solution. Will push it soon.

Copy link
Contributor

@demvlad demvlad May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe quickest to revert #819 , but i''m going offline until tomorrow.

I will check new psd code to work without #819 if i will not find normal solution

};
Expand Down
Loading