Resolved issue of magnitude computing at the spectrum charts #821
Conversation
|
""" WalkthroughThe internal implementations of Changes
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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 (
|
![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
🧹 Nitpick comments (1)
src/graph_spectrum_calc.js (1)
466-466: Consider explaining the 100Hz constantThe value
100in the noise index calculation appears to be a frequency threshold in Hz. Consider extracting this as a named constant to improve code readability.+const NOISE_LOW_END_FREQUENCY = 100; // Hz -const noiseLowEndIdx = 100 / maxFrequency * magnitudeLength; +const noiseLowEndIdx = NOISE_LOW_END_FREQUENCY / maxFrequency * magnitudeLength;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/graph_spectrum_calc.js(1 hunks)
🧰 Additional context used
🪛 ESLint
src/graph_spectrum_calc.js
[error] 467-468: Missing semicolon.
(semi)
🔇 Additional comments (4)
src/graph_spectrum_calc.js (4)
461-464: Improved FFT handling for spectrum calculation!This change correctly acknowledges that FFT output consists of complex value pairs (real, imaginary) representing a two-sided spectrum. Computing magnitudes for just one side of the spectrum is the right approach as the other side is redundant for real input signals.
471-479: Correctly calculating magnitude from complex valuesThis implementation properly computes the magnitude from the complex FFT output using
Math.hypot(re, im), which is equivalent tosqrt(re² + im²). This gives the true amplitude of each frequency component rather than using real and imaginary parts separately.
481-481: Properly scaling maxNoiseIdx to frequencyThe calculation now correctly scales the maximum noise index using the magnitude length instead of the full FFT length, which aligns with the reduced spectrum size.
486-488: Returning the correct magnitude array and lengthThe return object now includes the magnitudes array and updated length, ensuring downstream code receives properly normalized FFT data.
|
The last commit restores spectrum chart points count, because magnitudes have half points count than it were before. |
|
please post the BBL used for these screenshots. |
|
|
The GraphSpectrumCalc._dataLoadFrequencyVsX method what is used in throttle and rpm spectrum has this issue too. Will improve soon. |
|
The unfiltered pitch gyro by RPM spectrum before PR: It exists a small difference between separated re, im values before, and sqrt(re^2 + im^2) values after PR. |
|
just dropping this here, potentially unhelpful, or potentially miscalculated. Spoiler
|
|
Sorry, I can't add any valuable comments as not familiar with this math. But if you could find the source of this horisontal artefacts - it would be great) |
Yes, it is interesting. I'll figure it out. |
Seems, PTB has not spectrum by RPM. I have bin checked. The issues at the spectrums top are not just math error. But its have some reasons.
What can try to do:
|
needs to be clarified:
|
|
|
The true magnitude is more then separated Re and Im values. Therefore the "heat map" charts scaling was decreased, to save origin spectrum charts brightness. |
Will try to use fft with samples segmentation for current chart and will try PSD values too. |
|
This PR resolves some math issues only. But the charts view change looks as cosmetic. |
The current spectrum has a small issue. The FFT method returns complex output for two-side spectrum. It includes Re and Im complex values parts. The current release get half of complex FFT output and uses it as magnitude data directly. The current algorithm find maximum frequency amplitude value from Re and Im values too. This math is not proper. The spectrum chart shows Re and Im values with wrong 2x frequency resolution instead of true magnitudes.
This PR resolves this issue.
The test log:
btfl_all.txt
The unfiltered roll gyro spectrum before PR:
The unfiltered roll gyro spectrum by using this PR:
The unfiltered pitch gyro spectrum before PR:
The unfiltered pitch gyro spectrum by using this PR:
The different is not big for this spectrum, but this is correct computing.
Summary by CodeRabbit