Setting LNA gain sometimes fails

[luarvique]

NOTE: This is not about overriding default setGain()

I have found that setting LNA gain (RFGR) eventually stops working. When that happens, calls to SDRPlay APIv3 time out. This is usually preceded by a setFrequency() timeout, although the frequency still appears to update just fine:

: [WARNING] @@@ setFrequency(RF, 118800000)
: [WARNING] @@@ Setting RF frequency = 118800000
: [WARNING] Sample rate/decimation update timeout.
: [WARNING] @@@ writeSetting(rfgain_sel, 1)
: [WARNING] @@@ Updating rfgain_sel=1
: [WARNING] Gain reduction update timeout.

In the example above, the frequency jumped, but the rfgain_sel did not. While I do not know why this happens and how to fix it, it looks like some additional caution needs to be taken when calling SDRPlay APIv3. Maybe adding mandatory delay between API calls?

PS: Since SoapySDRPlay3 does not keep track of settings that failed to update, it will not try to set these settings again unless they are changed by the application. It might be a good idea to keep at least a bitmask of failed settings and try reapplying them the next time.

[fventuri]

@luarvique - thanks for reporting this bug.

Since you are familiar with the source code of the SoapySDRPlay3 module, you probably saw this code in the receive callback function (https://github.com/pothosware/SoapySDRPlay3/blob/master/Streaming.cpp#L88-L99):

    if (gr_changed == 0 && params->grChanged != 0)
    {
        gr_changed = params->grChanged;
    }
    if (rf_changed == 0 && params->rfChanged != 0)
    {
        rf_changed = params->rfChanged;
    }
    if (fs_changed == 0 && params->fsChanged != 0)
    {
        fs_changed = params->fsChanged;
    }

This is because changes to gains (or gain reductions), frequency, and sample rate are not instantaneous, i.e. they do not happen when sdrplay_api_Update() is executed, but at a later time; when they occur, the receive callback function is notified through the params argument via the three flags grChanged, rfChanged, and fsChanged.
To avoid these settings (like the receive frequency) to be changed too fast, i.e. before the previous change has completed, I added some wait loops like this one: https://github.com/pothosware/SoapySDRPlay3/blob/master/Settings.cpp#L511-L521 where I wait for some amount of time (the default is 500ms: https://github.com/pothosware/SoapySDRPlay3/blob/master/SoapySDRPlay.hpp#L301) before issuing those timeout messages.

I haven't run many tests to see if that default of 500ms is enough time in every scenario; perhaps you could temporarily increase it to say 1s (or even 2s) to see if the error message still shows up; it if does, I would enable debugging of the SDRplay API by commenting out line 356 and uncommenting line 357 in Settings.cpp (https://github.com/pothosware/SoapySDRPlay3/blob/master/Streaming.cpp#L356-L357), and then looking at the debug logs written by the API (you'll probably find them in /var/log/messages, or with journalctl depending on your Linux distribution) to see if they shed more light on this behavior.

Franco

[luarvique]

Well, I am seeing not just a timeout (it is more of a side effect), but the rfgain_sel value not being set at all.

As to wait delays, increasing that gr wait time to one second did not help either. It might be required to wait for some time BEFORE making a call rather than after, to prevent calls from happening too fast.

[luarvique]

While looking at the SoapySDRPlay3 source code, I have found something that probably does not affect the discussed issue, but may still need to be fixed:

// RX callback reporting changes to gain reduction, frequency, sample rate
int gr_changed;
int rf_changed;
int fs_changed;

I suppose, these should better be made volatile:

// RX callback reporting changes to gain reduction, frequency, sample rate
volatile int gr_changed;
volatile int rf_changed;
volatile int fs_changed;

To prevent compiler from optimizing their actual values away in those wait loops. Again, this is unrelated to the discussed problem (LNA state changes failing).

[luarvique]

And one more: the result of sdrplay_api_Update() is never checked for success in the Settings.cpp.

[luarvique]

Sorry for the previous comment about missing std::lock_guard: I have eventually found it in the Settings.cpp code. There is another issue there though: the code that waits for gr_changed to go up does so inside the lock_guarded function, essentially preventing rx_callback() from setting the gr_changed flag and whatever else.

[luarvique]

So, I might have fixed both the timeouts (for all settings) and the failures to set LNA state. Going to test it more, just in case, but the fix so far is to replace

     std::this_thread::sleep_for(std::chrono::milliseconds(1));

with

     _general_state_mutex.unlock();
     std::this_thread::sleep_for(std::chrono::milliseconds(1));
     _general_state_mutex.lock();

everywhere in Settings.c (and wherever else this happens). May even want to make it a small helper function, for consistency.

[luarvique]

See #59

[fventuri]

@luarvique - I did some reading this morning, and I think this kind of problem could be perhaps handled with a condition variable.

I followed the suggestions and example code from C++ reference (see here: https://en.cppreference.com/w/cpp/thread/condition_variable and here: https://en.cppreference.com/w/cpp/thread/condition_variable/wait_for).

I just pushed a first attempt using a condition variable to the new branch wait-for-value-change (https://github.com/pothosware/SoapySDRPlay3/tree/wait-for-value-change).

I ran a quick test using CubicSDR here, and it seems to work OK (but I didn't try it with OpenWebRX).

Please take a look, and let me know what you think and how it goes for you.

Franco

[luarvique]

@fventuri , you are missing the point. All the funny C++ soup you have added is not going to help you, since you are not releasing _general_state_mutex prior to the wait and thus blocking the callback from executing.

Instead of reading C++ references (never a healthy thing nowadays), look at your original code and at the changes I introduced in #59 - you will see what I mean.