pothosware / SoapySDRPlay3

Soapy SDR plugin for SDRPlay APIv3
https://github.com/pothosware/SoapySDRPlay3/wiki
MIT License
90 stars 15 forks source link

Preventing Soapy from messing frequency correction (ppm) #56

Closed luarvique closed 1 year ago

luarvique commented 1 year ago

The default setFrequency() implementation in Soapy will try to "distribute" given frequency over all available frequency components. So, it will set the RF component to the given frequency, then set the CORR component to 0.

This causes the frequency correction (aka ppm) to always reset after a setFrequency() call, breaking ppm functionality in such packages as OpenWebRX.

The proposed change makes setFrequency() and related methods default to the RF component. The CORR component is left alone and can be used to correct frequency settings on per-device basis.

fventuri commented 1 year ago

@luarvique - thanks for looking into it.

I spent a few minutes this morning looking at the SoapySDR API for setFrequency and I found this comment in lib/DeviceC.cpp (https://github.com/pothosware/SoapySDR/blob/master/include/SoapySDR/Device.hpp#L779-L781):

 * The default implementation of setFrequency() will tune the "RF"
 * component as close as possible to the requested center frequency.
 * Tuning inaccuracies will be compensated for with the "BB" component.

Based on this comment and your findings, I wonder if there is an inconsistency between that node and SoapySDR's default implementation of setFrequency().

Adding @guruofquality to hear what he thinks.

Franco

luarvique commented 1 year ago

There is no inconsistency, just Soapy being too smart for its own good about tuning frequency. You can see what happens at line 360 here:

https://github.com/pothosware/SoapySDR/blob/828847cefcb137f7c416f850a56ae26e447af694/lib/Device.cpp

guruofquality commented 1 year ago

Its just a default behavior thats probably worth overriding. You might want to:

luarvique commented 1 year ago

The CORR value does appear to be in kHz or something close, from my testing.

luarvique commented 1 year ago

BTW, I did consider removing CORR from the listFrequencies(), but decided against it, since it may break compatibility with some apps expecting to set it via "named" setFrequency() calls.

Overriding default "anonymous" setFrequency() felt like a better idea since we are dealing with specific hardware with well defined frequency API. There is no reason to use the default method (however "smart") if we know for sure how device likes its frequency set.

Same goes for the setGain() call by the way (see adjacent pull request).

fventuri commented 1 year ago

Thanks for your comments @luarvique and @guruofquality

I just pushed out a new branch called setFrequency (https://github.com/pothosware/SoapySDRPlay3/tree/setFrequency) where the 'generic' setFrequency() (the one without a specific frequency name) uses "RF" as the default. To be consistent I implemented the corresponding getFrequency().

I also added the get/set methods for frequency correction as suggested by Josh (they just call the get/set frequency methods with a name of "CORR").

I agree with @luarvique that we probably need to keep the "CORR" frequency name and functionality in place, since there are SDR client applications like CubicSDR that seem to still rely on it (see here: https://github.com/cjcliffe/CubicSDR/blob/master/src/sdr/SDRDeviceInfo.cpp#L181).

Anyhow please give it a try with OpenWebRX when you have time, and let me know how it goes.

Franco

luarvique commented 1 year ago

Please, do not forget to implement getFrequencyRange() as well! (see my patch)

fventuri commented 1 year ago

@luarvique - thanks! I just added that missing function.

Please give it a try now and let me know, Franco

luarvique commented 1 year ago

Currently away from home and ssh access to the OpenWebRX machine. Will test this Monday/Tuesday though. Sorry about the delay.

luarvique commented 1 year ago

Franco, I have checked out, built, and tested the setFrequency branch with the OpenWebRX. The frequency correction appears to work consistently with this branch.

fventuri commented 1 year ago

@luarvique - thanks for checking. I just merged the setFrequency branch into master (and created release 0.4.1 since it adds a few more public methods for frequency correction).

Franco

luarvique commented 1 year ago

Thanks =)