-
-
Notifications
You must be signed in to change notification settings - Fork 156
fix pr 819 breakage - this reverts returning samples, not splice of samples. #828
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.
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
Conversation
return {
samples : samples,
count : samplesCount,
};
WalkthroughThe update changes the behavior 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 (
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/graph_spectrum_calc.js (1)
332-335: Consider adding a clarifying comment about the array structure.For better code clarity, consider adding a comment to document that the returned
samplesarray contains valid data only up to thecountproperty, with unused elements beyond that.return { samples : samples, count : samplesCount, + // Note: samples array is pre-allocated and contains valid data only up to 'count' };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/graph_spectrum_calc.js(1 hunks)
🔇 Additional comments (1)
src/graph_spectrum_calc.js (1)
332-335:✅ Verification successful
Properly reverts to returning the full pre-allocated samples buffer.
This change restores the original behavior of returning the entire pre-allocated
samplesbuffer rather than a sliced version with only valid samples. Thecountproperty still indicates how many entries are valid.Run the following script to check for consistency with other similar functions in this file:
🏁 Script executed:
#!/bin/bash # Description: Verify how other functions in the file handle returning samples arrays echo "Checking how similar functions return sample arrays:" rg -A 5 "return \{" src/graph_spectrum_calc.jsLength of output: 1205
Approval: Consistent return of full buffer with count
This change correctly restores the original behavior of returning the full pre-allocated
samplesbuffer while indicating valid entries via thecountproperty. In this file, pure-spectrum functions return the full buffer with acount, and slicing is only used when aligning multiple parallel arrays (e.g.,vsValuesorpiderror/setpoint). No further changes are required.
|
Does exist some error with slice? By other hand, there are no error without slice too. |
|
https://discord.com/channels/868013470023548938/1371508791635476491 i'm open to other fixes., make a PR if another solution is better, more proper. multiple reports of master broken. here is log for testing: BTFL_BLACKBOX_LOG_20250510_181940_STELLARH7DEV_killing_lipo_need_voltage_telem.BBL.zip |
@nerdCopter |
|
Yes, the simple spectrum does not work. |
|
i did not make a branch 🤕 |
|
It looks strange at the first look, but the FFT.simple call hangs with true data size. Will study. |



graph_spectrum_calc.js:fixes breakage cause by #819
Summary by CodeRabbit