Nicer panoramic spectrum zoom #245
Conversation
Also switch to fast RTT thanks to my driver changes, short buffer size, and new RTT meaning in suscan that excludes actual retune time. Now, the RTT is just to account for latency from buffering and transfers.
Based on new suscan logic where the discarded duration is the measured retune time plus the user specified latency.
Forcing the two to be equal didn't really make sense. With this independence, we can have a bunch of small FFTs of at low sample rates combined into a bigger spectrum. Alternatively, we might want an FFT size bigger than the spectrum size when the sample rate is high, just so the capture duration at each frequency is reasonably long.
Capture at least one ms of data at each frequency.
The old logic didn't handle zoom nicely because scaledPsdAccum was supposed to represent the full frequency range of the source information while being at the target bin width. In case of zoom, the source frequency span is bigger than the target frequency span, so it would cut off data at its end. This cutting off of data at the end caused the end of the resulting PSD to always have no data even though the source contained it. This new scaling logic is simpler, performs calculations in place (avoiding the need for the scaled accumulator buffer), and only performs calculations on the portion of the source frequency range that is within the target frequency range. Note that this new implementation no longer interpolates between frequency bins, for simplicity and performance's sake. Instead, it just takes the nearest bin. However, I may add back such interpolation if we later decide it's important enough to justify the performance penalty.
Helps preserve more frequency resolution for zoom
For nice frequency zoom. TODO: make it optional
Shouldn't normally be needed unless something went wrong, though I encountered a segfault here when I introduced a bug elsewhere. Minimal overhead to add safety, so I'll just add a check.
Avoid using overly large spectrum sizes with redundant bins when zooming in, since it's just wasted computation. Also make the spectrum size a power of 2 to keep GLWaterfall's downsampling logic happy. Scanner: make frequency resolution a constant
Allow the waterfall's internal representation of the full range to change size when the waterfall is restarted with a different range.
With the new partial update waterfall design, the waterfall center frequency remains constant and accurate.
|
One known issue at this moment: when you stop the scanner and then zoom into the last frequency range fed into the waterfall by the scanner (pointed to by |
|
I am afraid this is crashing SigDigger in my machine during startup: |
|
It seems that |
|
Fixed, now I am seeing the panoramic spectrum just fine. It is almost like magic, being able to zoom in and out with OpenGL enabled here and also with the peace of mind of having a dedicated API to update partial portions of the FFT. Awesome! It will take a while for me to review all these changes, but this looks good so far. |
|
Great, glad you like it. Thanks for catching the waterfall pointer initialization issue, I’ll add a commit for that. I also started fixing that UAF I mentioned last night, I’ll upload a fix for that once ready. |
|
Nah don't worry about the waterfall pointer. I prefixed everything by m_ in
the same commit. I have a copy of your feature branch under SigDigger to
keep working at home.
El jue., 16 may. 2024 14:20, Sultan Qasim Khan ***@***.***>
escribió:
… Great, glad you like it. Thanks for catching the waterfall pointer
initialization issue, I’ll add a commit for that. I also started fixing
that UAF I mentioned last night, I’ll upload a fix for that once ready.
—
Reply to this email directly, view it on GitHub
<#245 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEVETY6SIE47BPUYLQUC4TZCSQB7AVCNFSM6AAAAABHY7DL6KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJVGEYDENJZHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Ah, I see. I'll rebase on your https://github.com/BatchDrake/SigDigger/commits/sultanqasim-develop/ branch. |
Defer destruction of the Scanner class till we restart the panoramic spectrum so that the waterfall's reference to the Scanner's spectrum view PSD data remains valid.
|
Building on top of your commit, I fixed the use-after-free issue and a couple other bugs. |
Requires a corresponding change in suscan to permit it
|
You will also need BatchDrake/suscan#87 to support the last commit about non-hopping startup |
|
Just a note, if I zoom in with say a 90Mhz - 100Mhz range and then try to zoom out with the scroll wheel, it doesn't actually seem to scroll out to where it first started. I have to hit stop then start to get the full range back in view within the fft/waterfall window. Also selected MHz doesn't seem to match what I'm selecting. I'm probably getting ahead of things that are still being worked out and/or doing something wrong on my end. |
|
Probably, since changes for this feature span SigDigger, Susan and
SuWidgets. I haven't finished the review yet.
El jue., 23 may. 2024 5:24, alphafox02 ***@***.***> escribió:
… Just a note, if I zoom in with say a 90Mhz - 100Mhz range and then try to
zoom out with the scroll wheel, it doesn't actually seem to scroll out to
where it first started. I have to hit stop then start to get the full range
back in view within the fft/waterfall window. Also selected MHz doesn't
seem to match what I'm selecting. I'm probably getting ahead of things that
are still being worked out and/or doing something wrong on my end.
—
Reply to this email directly, view it on GitHub
<#245 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEVET6TKTYT7WC7GAICM3LZDVOOZAVCNFSM6AAAAABHY7DL6KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRWGE2TSNJUGM>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Seems strange; are you sure you have the right branches built? BatchDrake’s development branches of SuScan/SuWidgets/Sigutils + this branch of SigDigger should work. I’ll message you a video of how it should be behaving. |
|
I changed the following just now and pointed the relevant section of blsd to it. It was late last night, so when I did build it successfully I had actually reverted to it building the standard version and didn't realize it. Now that I'm back to building the correct branch(s?), I get the |
|
Searching seems to point to the |
|
I was able to reproduce the error stopping the scanner on Linux. I'll look into this when I have a moment. |
We won't be using it again, and deleting it is necessary to properly stop it, so just delete it. We only need to hold onto the spectrum views used by the panoramic dialog.
|
Deleting the analyzer on stop should make the stopping work like before from suscan's perspective, and there was no need to keep it around anyway, since we just needed the spectrum views in the scanner. Now stopping it works properly on Linux. |
I’m going to try the change suggested here to the .pro file https://forum.qt.io/topic/84269/can-i-use-clang-with-std-c-17-in-qmake |
|
Modifying the SigDigger.pro to CONFIG += c++1z allows me to build this PR. May need to account for this need on 22.04 or other systems. |
Scanner now uses std::clamp from C++17
Hopefully my last patch will fix build for you |
Red Hat family distributions put libraries in `/usr/lib64` instead of `/usr/lib`. This includes sigutils getting put in `$DEPLOYROOT/usr/lib64`.
|
I also put up an unrelated change to fix build on Fedora |
For Red Hat/Fedora
In case the user changed the frequency range right before clicking start on the panoramic spectrum, a signal to update the view range will make its way here before the first PSD arrives. I'm keeping the logic to set the spectrum view range based off the sample rate in fixed frequency mode when the sample rate is known so that we are not discarding potentially useful data about what's happening on nearby out-of-view channels when zoomed in. It isn't necessary to do this (in fact its more efficient to only keep the frequency range we are currently looking at in the spectrum view), but maybe someone would like to zoom out and see what was happening on nearby out-of-view frequencies while they were zoomed in.
|
Sorry about my flip-flopping force pushes regarding the last commit around how to handle frequency range updates in non-hopping mode. After some initial thought I removed the logic to base the spectrum view range on the sample rate altogether, but later decided to bring it back because there is one situation where it could be useful: looking at what happened on channels outside the zoomed view when zooming back out or panning in the OpenGL waterfall. Anyway, this is working well for me on Linux and Mac and should be ready to be merged. Also have a look at my latest SuWidgets pull request; I have a couple bug fixes there. |
|
With the change to the SigDigger.pro it builds without issue on 22.04 now with stock qt5. Just built it by changing blsd to pull dist-common from my fork of your SigDigger and all lines putting to develop on your repos. Hope that makes sense. |
| UIMediator::getPanSpectrumZoomRange(qint64 &min, qint64 &max, bool &noHop) const | ||
| { | ||
| if (!this->m_ui->panoramicDialog->invalidRange()) { | ||
| this->m_ui->panoramicDialog->getZoomRange(min, max, noHop); |
There was a problem hiding this comment.
Now that I see this, next time you see a class with private properties starting with m_, try not to use this-> unless necessary. I used to abuse this-> in the past (perhaps because too much influence from C) I eventually made the code for big classes unreadable. The solution was, since I still wanted to clearly see in a glimpse which variables were properties, which local and which global, to remove the this-> (even for methods) and prefix private members (not methods) with m_ (and globals with g_, although this is something I was already doing in C). You will see that, once in a while, when I have to work in an existing class written in old style, I leverage the changes to rewrite them in the new style.
So, no need for you to change that (I will do it at some point in the future), just wanted to let you know (and hopefully make your life easier)!
|
Hi, I have just tested the changes. Some issues (some of them may be yours, some mine):
|
|
Thanks for your testing,
|
|
One other thing: because I changed the meaning of "RTT", it would be good to redetermine the default/optimal "RTT" values for the various SDRs. I did this for the RTL-SDR and USRP, though the same should also be done for the other listed SDRs that I don't have personally. |
So that it can use slow rate logic for long RTTs
Behaviour change at BatchDrake's request
|
I figured out the full device range issue: it was because the frequency spin box wasn't emitting |
Agreed, perhaps we should call it "Device settle time"?
Yeah, just verified it. I will address it myself, don't worry about it.
I couldn't reproduce it any longer. Perhaps it was the uninitialized variable we discussed earlier.
Yes, I actually prefer the extension of the values on the edges. In a bayesian sense, if I had to make a bet on the power level adjacent to the spectrum edges, I'd say "something close to the value on the edge". I cannot test the code right now (I didn't bring any SDR with me this time), but I will test it once I am back home. Thanks a lot! |
|
Testing now again |
Needs to be paired with changes to SuWidgets, and some changes I made to suscan also help