-
Notifications
You must be signed in to change notification settings - Fork 5
Fix multirange setup #8
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
base: main
Are you sure you want to change the base?
Conversation
Appreciated, will review. Not a fan of the multiplication arithmetic inside array declarations and dereferencing, especially with loop variable counters involved, and 16-bit integers (signedness at that point is neither a mitigating nor worsening condition during overflow...). Negligible impact here, just a pedantic observation. Keep in mind this is upstream HackRF code (that specific part anyway), so, if it is broken here, it is also broken there. (@straithe) Please bug them here: Make sure you reference this project. Happy to take fixes in at a faster pace than them ;-) We already suggested them to merge the entirety of sweeper into HackRF's repo: Other issues: |
Thanks for the ping! I've let the GSG team know. |
Stale pull request message |
@straithe Is the HackRF tool codebase a dead project? We see this issue remains unsolved for quite some time (as are other open issues). |
This is a third-party code base, if you are looking for the GSG version please look at https://github.com/greatscottgadgets/hackrf/ |
@straithe I meant your codebase, precisely. This user reported to us a bug in your project, just like several other shortcomings and problems explicitly involving the hackrf_sweep implementation in your upstream sources. Since you "let the GSG team know" back in January 23rd, has there been any materialized effort in fixing the problem? I would haphazard it does not take months to triage and fix. |
@unjambonakap File an issue in upstream, I will finally see if I can squeeze this in along some other fixes that upstream has been consistently neglecting for a while. |
I am not a GitHub owner of any of the Great Scott Gadgets repositories or any repositories for HackRF at this time. If you have concerns with any of those repositories I suggest opening an issue on the relevant repository. |
@unjambonakap A bit of an update: please test your proposed changes like so:
I will validate your test outputs, validate the arithmetic you are using in the array indexes (we don't want a FFT-triggered remotely exploitable out of bounds array write, right? :>) and merge. @straithe GSG had a cumulative profit between DARPA and Kickstarter/crowdfunding for the HackRF alone in the range of 1mil USD. Plenty to hire outside help if you can't cope with the influx of reported issues, which, by the way, are referenced right here in the first response to this PR. I'm not interested in the minutiae of your/their staffing woes, and I don't have time to entertain open source politics, or fix someone else's bugs for that matter. I pointed those problems out as they were already reported to upstream, and you also verbalized "letting the GSG team know", so if you have such proximity with them, it is only fair to expect that they would have listened to you and the half a dozen long standing reported issues. |
@sbrptdev2 FFT triggered remote exploit - that would be a fun CTF challenge :) |
No problem, the important thing is to prove your PR does not introduce unintended effects that break or change how existent features work. You don't need a NIST calibrated RF gen either, just make sure the output is stable (and parameters are disclosed so we can replicate it). Even in a perfect, non-existent test scenario odds are internal noise and other factors will polute the FFT output. Try to adjust settings (especially gain) so that you minimize that measurement noise. Let me know if you need help/assistance. |
All the results are in https://gist.github.com/unjambonakap/05b15f01a819fd20cc2c00c0ecbfbe40 Command is Sines bolded (edit - starred :))
|
Excellent, will review and report back. |
I will be merging this soon, sorry for the delay. Excellent PR, again, just for the testing provided. |
IFFT mode does not work with multirange but TEXT at least does