Skip to content
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

Override setGain and getGainRange to avoid changing RFGR. #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dlaw
Copy link

@dlaw dlaw commented Jan 3, 2021

SDRPlay's "RFGR" is actually a mode selector for the front-end LNA. It does not have units of dB. (In fact, the resulting gain is frequency-dependent.) As a result, the default gain allocation algorithm does not apply and only IFGR should be used for automatic adjustment of the overall system gain.

(See pothosware/SoapySDRPlay2#60 for previous discussion, https://www.sdrplay.com/docs/SDRplay_SDR_API_Specification.pdf section 5.3 for the definition of RFGR.)

SDRPlay's "RFGR" is actually a mode selector for the front-end LNA.
It does *not* have units of dB. (In fact, it is frequency-dependent.)
As a result, the default gain allocation algorithm does not apply and
only IFGR should be used for adjusting the overall system gain.
@dlaw
Copy link
Author

dlaw commented Jan 3, 2021

FYI, there is some discussion going on regarding the best path forward for #26, cjcliffe/CubicSDR#825, and the new-gain-controls branch.

However, I believe that this PR is still appropriate for an immediate merge into master -- it is a standalone fix for a bug in SoapySDRPlay behavior, and it is independent of any future changes to the gain controls.

@dlaw
Copy link
Author

dlaw commented Apr 29, 2021

Would it be possible to get this PR merged now, even as we continue discussion about the best long term solution for gains in the SDRPlay driver?

This change does not break any existing use cases, and I think everyone agrees that setting the overall system gain should not treat the LNA state number as a value in dB.

@fventuri
Copy link
Collaborator

@dlaw
Sorry for answering you only now, but during the day I have to work my regular job, so I can reply to these comments only after dinner (or on weekends).

I think that the "generic" getGain() and setGain() should be defined consistently, i.e. if a user sets the "generic" (to be somehow defined) gain using setGain(), when they later call getGain() it should return a value somewhat close to what they set (or at least 'semantically' close).
Looking at the current code for setGain() in the new-gain-controls branch (https://github.com/pothosware/SoapySDRPlay3/blob/new-gain-controls/Settings.cpp#L637), I see that it is defined as a combination of the IF gain (or gain reduction) and the RF gain (or LNA state).

I looked at the commit history for that code and I see that that approach was introduced by @SDRplay almost a year ago with this commit (1da639a#diff-d3c542b2c3cb1d01a894f311212c4cf20d0fe52be1ed128b5ff2c475507e834f).
Since it was a year ago I honestly don't remember much about the rationale for this approach (perhaps it is similar to what SDRuno does, not sure), but I would like to hear @SDRplay's opinion of what he thinks the behavior of the "generic" getGain() and setGain() should be, before going in one direction or the other.

Franco

@dlaw
Copy link
Author

dlaw commented Apr 30, 2021

I think this PR makes sense to merge immediately as a stopgap measure while we continue the discussion about new-gain-controls over in #27. The current behavior of setGain is so broken as to be unusable. This PR is the minimal change to fix the incorrect behavior.

I have some application (in my case, trunk-recorder) which is not capable of setting named gains. My only option is to say I want a "generic" gain of X dB. The current driver implementation of setGain will change my LNA state and IFGR so that the sum of the 0-9 LNA state and 22-59 LNA state is X. This is basically guaranteed to set the LNA state to 9, making the application unusable.

For now -- until we have some smarter way to handle the LNA state as an RF gain in decibels -- I think the correct behavior is for generic setGain to set the IFGR value and getGain to return the IFGR value, leaving LNA state alone.

@fventuri
Copy link
Collaborator

@dlaw
I think there is something I am missing with your setup over there.

You are saying that your application trunk-recorder can only use a "generic" setGain() (i.e. one without a named gain control like IF or RF). Since that "generic" setGain() is not currently implemented in the master branch, I am assuming you are using the new-gain-controls branch (please correct me if I am wrong), where the implementation of the "generic" setGain() should not have the problem of say setting the LNA state always to 9, since it is computed from a set of tables (https://github.com/pothosware/SoapySDRPlay3/blob/new-gain-controls/Settings.cpp#L642-L662) from a value in the range [0-28] (if I counted them correctly), which means that if you are using an RSPduo and call setGain(...,14), you would get an IF=45 and LNAstate=5, which at a quick glance seem reasonable half-the-way kind of settings.

Franco

@dlaw
Copy link
Author

dlaw commented Apr 30, 2021

@fventuri I have this problem with master branch. SoapySDR provides default setGain and getGain implementations which have the behavior I describe unless they are overridden.

Here is the undesired code which forces LNA state to 9 on master:
https://github.com/pothosware/SoapySDR/blob/master/lib/Device.cpp#L294

Here is an old issue describing the problem:
pothosware/SoapySDRPlay2#60

Since it seems like we still have quite some discussion ahead of us on new-gain-controls, I think it makes sense to prevent generic setGain from messing up the LNA state in the interim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants