Lock fix

[luarvique]

Merged with the latest upstream code. Added better locking granularity to the callback. The fix for intermittently failing sdrplay_api_Update() call can be found in a separate pull request.

[fventuri]

@luarvique - I did some research on the C++ attribute 'volatile' tonight, and it doesn't seem to help much with multithreading (see here for instance: https://stackoverflow.com/a/4558031).
Perhaps we can try to investigate the cause of those update timeouts using the current version of the master branch, and discuss a good way to address them.

Franco

[luarvique]

The 'volatile' keyword makes sure that the compiler does not optimize accesses to these variables away, i.e. if you do something like

volatile int done;
...
while(!done) { do_stuff; }

the loop will actually read "done" variable on every pass. The use of "volatile" keywords is helpful, but it is not what fixes the threading issue. Two separate changes fix the threading issue in the proposed patch:

1. In SoapySDRPlay::rx_callback(), the flags are now protected with the _general_state_mutex to prevent race condition between rx_callback() and whatever functions are resetting these flags in Settings.cpp:

if (params->grChanged != 0 || params->rfChanged != 0 || params->fsChanged != 0)
{
        std::lock_guard <std::mutex> lock(_general_state_mutex);

        ...
}

2. In Settings.cpp, there is now a universal function that implements a wait with the _general_state_mutex released, in order to allow rx_callback() a chance to set flags:

void SoapySDRPlay::waitForDevice(int msec)
{
    _general_state_mutex.unlock();
    std::this_thread::sleep_for(std::chrono::milliseconds(msec));
    _general_state_mutex.lock();
}

If you call sleep_for() while holding the mutex, the rx_callback() will simply get stuck for the entire duration of sleep_for(), as many times as you call it. Here is what a typical wait for such a flag looks like now, in Settings.cpp:

      for (int i = 0; (i < updateTimeout) && (gr_changed == 0) ; ++i)
      {
         waitForDevice(1);
      }

This code is not very different from yours, except that every time it puts thread to sleep for 1msec, it releases the mutex first, to let rx_callback() set gr_changed safely.

I have forked SoapySDRPlay3 code and implemented all the changes I deemed necessary, including the above lock fix, multiple sdrplay_api_Update() retries, and the removal of RFGR gain from the overall gain control. The modified code is available here:

https://github.com/luarvique/SoapySDRPlay3/tree/develop

You are welcome to look at it, use it, and import any of the changes into your own code. Send me a private email if you would like access to an ARM64-based receiver running this code.

[ast]

Regarding volatile, I would recommend using a std::atomic_flag and maybe memory order relaxed.

I have recently forked and rewritten the SoapyAirspy and SoapyAirspyHF drivers. In the process I wrote a new ring buffer class specifically for writing Soapy drivers. Maybe that could be of interest?

You can take a look at github.com/ast/SoapyAirspy and github.com/ast/SoapyRingbuffer

[fmoessbauer]

As already said in #59 (comment) and also pointed out by @ast and @fventuri , the volatile should be replaced by std atomics. In C++ the volatile keyword is really only needed when working with memory that is externally changed (e.g. by signals). In fact, the volatile keyword is not sufficient to guarantee memory ordering between threads. For details, see https://en.cppreference.com/w/cpp/language/cv

[luarvique]

In fact, the volatile keyword is not sufficient to guarantee memory ordering between threads.

The 'volatile' keyword is not used to guarantee memory ordering between threads in the changes being discussed. It has been said so earlier in the comments.

[ast]

Strawman lol.

I apologize to everyone involved that I got involved in this thread with my unsolicited advice, but I got a notification from pothosware and couldn't keep my mouth shut.

I will leave you with a final piece of advice: When using sleep for "mutex lock slack" you should have a pretty damn good argument why you are not using a condition variable.

[luarvique]

I will leave you with a final piece of advice: When using sleep for "mutex lock slack" you should have a pretty damn good argument why you are not using a condition variable.

It is not a particularly good argument, but I did not want to refactor the original code too much. The goal was to fix, not to rewrite.

[fventuri]

@ast, @fmoessbauer - First of all I really appreciate your interesting and valuable comments in this discussion.

Regarding the use of a condition variable (which I agree should be the way to go), sometime ago a created a branch called wait-for-value-change (https://github.com/pothosware/SoapySDRPlay3/tree/wait-for-value-change), where I implemented the conditional variable approach (see here for instance: https://github.com/pothosware/SoapySDRPlay3/blob/wait-for-value-change/Settings.cpp#L529-L532).
I write about it back on 9/17 (#58 (comment)), but I suspect I was the only one who tried it.
A few days ago I updated the code in that branch with the code from master so it has all the commits from master, and tonight I ran it for a bit again on my Linux desktop (x86_64) without seeing any problem.
If you run it, you'll see Sample rate update timeout. messages; those are due to a known bug in the current SDRplay API for Linux (version 3.07), where the flag params->fsChanged is never set; SDRplay is aware of the problem, and they are working on a fix.

@ast, I took a look at your very fast ring buffer, and it seems similar to a ring buffer implementation I wrote a few months ago (https://github.com/fventuri/rsp_snd/blob/main/src/ringbuffer.cpp), which follows more or less what Jakob Ketterl did in the csdr library (https://github.com/jketterl/csdr/blob/develop/src/lib/ringbuffer.cpp).
One thing I am not really sure is if those mmap and mremap functions exist under Windows (I use Linux here), since Windows is also one of the OS's that people use to run SoapySDR/SoapySDRPlay3.

Franco

[ast]

@fventuri thank you for the additional information! That's very interesting!

The APIs for double mapping memory exist under Windows and macOS (but they are different, of course)

ps. I think you only need one mmap and one mremap.

Albin

[fventuri]

@ast - Albin, thanks for the useful info about Windows and Mac.
GNU Radio is using something similar too: https://www.gnuradio.org/blog/2017-01-05-buffers/

All the implementations of this virtual memory approach I saw use a total of three mmap/mremap calls (see here for instance: https://abhinavag.medium.com/a-fast-circular-ring-buffer-4d102ef4d4a3); I didn't know it could be done with just two.

Franco

[ast]

I'm not sure but your approach with using mremap instead of mmap with a file descriptor was new to me. I Googled around and saw some indications that one mmap and one mremap could be sufficient. I will experiment with it.

Yes gnuradio uses the same idea and you can probably find code for windows and macOS memory mapping there.