-
Notifications
You must be signed in to change notification settings - Fork 50
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
Multi-RX: Use current/max RX and TX sample rates for the equalizer. #142
Conversation
@drowe67, my understanding from emails Jose sent is that there is noise in the received audio unless the curves/volume levels in the filters are 0dB. This PR is to ensure that the filters use the same sample rates as what the various modes expect (e.g. possibly higher than 8K). |
@drowe67, this one is ready to merge. |
We have needed equaliser support for 2020 at Fs=16kHz for some time, so this PR is a step in the right direction well done 👍 I'm not sure what the current behaivour of the EQ is in 2020, probably the frequency scaling is wrong. I can't recall any strange noises. I also can't recall any noises when the EQ is used in the 8 kHz modes. As we discussed recently, this feature has been around for a long time and has been in constant use.
Also I don't think this will work correctly in multi-rx mode, as I think |
Also on the graphs the X axis (freq) only goes out to 3000 Hz. Although it's probably no big deal, even in 16 kHz mode there is not much energy above 3000 Hz, and most of the EQ required is probably at the lower frequencies. |
@drowe67, unfortunately I couldn't duplicate it on my Mac but some of the Windows users were seeing it. This PR was merged into ms-optimization to get some runtime and I haven't heard anything about the noise still being there, FWIW.
MainFrame::OnTimer() recreates the filters every 100ms using the below code:
In fact, the audio from the running session changes in real time if you have the Filters window open and adjust the settings in there. 👍 Presumably the filters will readjust on RX mode changes with about 100ms lag. |
Hi @tmiw - I think the filters are only redesigned if the used tweaks the controls. This sets I think we also trigger new filter designs at start up as well. |
Another hint from a recent email from Jose:
I think he's suggesting this issue crept in with 1.5.3? |
Oops, you're right. I just pushed a couple of commits to redo the filters on TX and RX mode changes.
That's when multi-RX became official, so it would make sense that the issue started appearing when multi-RX did. |
Some more info on this one from a recent email from Jose:
Can we please get some more user feedback/test results from this one, along the lines that I mentioned a few days ago? While I can see this PR is heading in the right direction (e.g. proper EQ support for 2020, multi-rx issues), I would prefer not to merge until we are sure it has (i) fixed a bug/improved performance and (ii) is not causing any new problems. Useful tests would be asking some one to run through all modes while trying the EQ in Tx and Rx, or popping up a test build for Jose and others to try. I'll take an action to try a few tests myself too. |
@drowe67, I received further clarification from Jose LU5DKI. It sounds like this PR does resolve the analog buzzing/noise issue for him, FWIW. |
Hi @tmiw - I set up a full duplex sound card loopback and did some basic tests of the EQ functionality on this one:
No odd sounds. I had multi-rx running but as it was synced up I guess it wasn't having much effect. |
@drowe67, I think it was originally showing up either with the Analog button pressed but with squelch off. I just tried it with the latter + band noise from my SDR program and I didn't hear anything weird either. |
@drowe67, I haven't heard anything indicating that this is still an issue (or that the most recent build caused another issue), so we can go ahead and merge. |
@drowe67, I just resolved conflicts so this should now be okay to merge. 👍 |
Looks like something that was missed when multi-RX was originally implemented. I think it helps but I'll include this PR in the next build to confirm.