Fix for issue with incorrect center frequency due to changing while active

[cjcliffe]

Appears to be working here without issue on macOS, Ubuntu Linux and Win10

[argilo]

@cjcliffe This change causes a pause of more than one second every time the frequency is changed. In Gqrx that is quite annoying:

What is the problem you were trying to solve? I suspect there is a better way than shutting down the receiver before changing frequencies.

[zuckschwerdt]

This is indeed needed and fixes just the setFrequency(), all other commands will still fail when a stream is running. The rtlsdr_read_async() needs to end before issuing any commands as the running usb transfer blocks access. Stopping rtlsdr_read_async() is not ideal as this cancels the usb stream and tears down all buffers. https://github.com/osmocom/rtl-sdr/blob/master/src/librtlsdr.c#L1811 and https://github.com/osmocom/rtl-sdr/blob/master/src/librtlsdr.c#L1733 It might be an option to optimize the async read loop in librtlsdr. Maybe it's possible to issue commands just before resubmitting the usb transfer https://github.com/osmocom/rtl-sdr/blob/master/src/librtlsdr.c#L1708 or more lightweightly stop and restart the transfer there. Not sure, in my cursory test it wasn't working ;)

[argilo]

This is indeed needed

Are you sure about that? Why doesn't gr-osmosdr need to do this? In the past 8 years of using RTL-SDR dongles I've never experienced a problem with setting the frequency during streaming.

[argilo]

all other commands will still fail when a stream is running

I don't see this problem either. I'm able to change the gain during streaming without issue.

[zuckschwerdt]

Are you using rtlsdr_read_async() or a loop with rtlsdr_read_sync()? For me the libusb_bulk_transfer() in the sync case works, but the running libusb_submit_transfer() in the async case dosn't.

[argilo]

gr-osmosdr uses rtlsdr_read_async: https://github.com/osmocom/gr-osmosdr/blob/9b386707d81ac8117a6c26011a757db40aa505ea/lib/rtl/rtl_source_c.cc#L323

[zuckschwerdt]

Maybe something about the libusb support on different platforms. I'll try a minimal example on some and report back soon.

[argilo]

Thanks. A minimal test case that reproduces the problem would be very helpful.

[zuckschwerdt]

See https://github.com/zuckschwerdt/rtl-sdr i.e. https://github.com/zuckschwerdt/rtl-sdr/commit/1a785de18a58f8e9e2d83d76e6e140294b0cd957 Tested on Debian Buster (libusb-1.0, version 1.0.22) and MacOS (libusb-1.0, version 1.0.24):

Reading samples in async mode...
rtlsdr_demod_write_reg failed with -6
rtlsdr_demod_read_reg failed with -6
r82xx_write: i2c wr failed=-6 reg=17 len=1
r82xx_set_freq: failed=-6
rtlsdr_demod_write_reg failed with -6
rtlsdr_demod_read_reg failed with -6
WARNING: Failed to set center freq.

Can you please test in your setup and give some details?

[argilo]

Aha, now I understand. I've encountered this issue as well. The rtl-sdr library does not allow rtlsdr_* function calls from inside the async callback function.

[zuckschwerdt]

I think that's generally the case while rtlsdr_read_async is running, not just on the callback. There is a libusb thread running and concurrent access to librtlsdr might be even worse.

[argilo]

I think that's generally the case while rtlsdr_read_async is running, not just on the callback.

I don't believe so. Applications like Gqrx call rtlsdr_* functions all the time during streaming, and it is not an issue.

[argilo]

From the libusb docs (http://libusb.sourceforge.net/api-1.0/group__libusb__asyncio.html):

Important Note: The user-specified callback is called from an event handling context. It is therefore important that no calls are made into libusb that will attempt to perform any event handling. Examples of such functions are any listed in the synchronous API and any of the blocking functions that retrieve USB descriptors.

The rtl-sdr library uses libusb's synchronous API in functions like rtlsdr_get_center_freq, so it's not possible to call those from within the callback.

[zuckschwerdt]

I also experience these errors when randomly calling the API outside the callback when the async is running: https://github.com/zuckschwerdt/rtl-sdr/tree/random_set_freq i.e. https://github.com/zuckschwerdt/rtl-sdr/commit/21c12f37c2addfd5006271fc9481576ecc2a1c1f

Reading samples in async mode...
rtlsdr_callback() enter
rtlsdr_callback() leave
rtlsdr_callback() enter
rtlsdr_callback() leave
rtlsdr_callback() enter
rtlsdr_callback() leave
rtlsdr_callback() enter
rtlsdr_callback() leave
rtlsdr_callback() enter
rtlsdr_callback() leave
rtlsdr_demod_write_reg failed with -6
rtlsdr_demod_read_reg failed with -6
r82xx_write: i2c wr failed=-6 reg=17 len=1
r82xx_set_freq: failed=-6
rtlsdr_demod_write_reg failed with -6
rtlsdr_demod_read_reg failed with -6
WARNING: Failed to set center freq.

Can you again test if this works for you?

[argilo]

I doubt that libusb functions are safe to call from within a signal handler.

Can you try the same from a thread? I expect that will work fine.

[zuckschwerdt]

The signal handler runs on the main thread (any eligible thread but there aren't any other here), we know it interrupted the rtlsdr_read_async thus likely libusb_handle_events_timeout_completed and we hope not to call anything non reentrant on the way. librtlsdr is not thread-safe, calling from another thread would be trouble and still interrupt the same piece of libusb. (threads only ever make things worse…) I did once by accident run rtlsdr_read_async in a worker thread and issue calls to the dev from the main thread and it wasn't pretty.

[argilo]

calling from another thread would [...] still interrupt the same piece of libusb

I don't believe so, because libusb itself is thread-safe.

[argilo]

SoapyRTLSDR itself uses a separate thread to handle streaming. Its callback just notifies another thread that data is ready, so there shouldn't be any calls into rtlsdr_* from within the callback.

Do you know what issue originally prompted this pull request?

[zuckschwerdt]

In effect you mean that calling libusb's sync API, e.g. libusb_control_transfer (on the same handle) while in async processing might be safe? Isn't sync built on async and that relies on select()-- running two threads on select() of the same FDs will break.

Might be worth to ponder if we could switch librtlsdr completly to the async libusb style? I guess it would need locks to keep the rtlsdr sync API then. Just shifting the problem perhaps.

I don't know about this change, but it's in line with my experience: don't call librtlsdr while async is running.

[argilo]

In effect you mean that calling libusb's sync API, e.g. libusb_control_transfer (on the same handle) while in async processing might be safe?

Yes, as far as I know that is safe.

Might be worth to ponder if we could switch librtlsdr completly to the async libusb style?

Some other (abandoned) work in this area: https://lists.osmocom.org/pipermail/osmocom-sdr/2015-February/000023.html

it's in line with my experience: don't call librtlsdr while async is running

GUI applications (like Gqrx) do this very commonly. I think the only thing you need to avoid is calling librtlsdr from within the async callback itself.

[zuckschwerdt]

If we are careful not to touch anything the rtlsdr_read_async uses then issuing calls to librtlsdr from one(!) other thread might just work then. Thanks for pointing this out so adamantly :) Which also means, since the async read is running in a thread for SoapyRTLSDR, there should never have been a problem to prompt this change. Strange, I see your point now.

[argilo]

there should never have been a problem to prompt this change

Well, not due to calling librtlsdr from within a callback at least. I'd like to know what the original problem was, because I suspect there's a better solution than shutting off streaming every time a frequency change is requested.

[zuckschwerdt]

Here is a minimal threaded example working fine so far: https://github.com/zuckschwerdt/rtl-sdr/tree/thread_set_freq

[argilo]

Your threaded example works for me as well.

issuing calls to librtlsdr from one(!) other thread might just work

I expect this is correct. Although librtlsdr makes no attempt to prevent concurrent access to its rtlsdr_dev struct, the fields that are used within its _libusb_callback are not used in any of the synchronous functions, and the libusb calls it makes are inherently thread-safe.

[zuckschwerdt]

@cjcliffe any insight on this change? @guruofquality I'm going to revert this change in a few days. Extensive testing shows no benefit from this PR.

[zuckschwerdt]

Commit 60b41d5 reverts this PR. The above discussion shows that rtlsdr_set_center_freq() is safe to use from a different thread than the one blocked on rtlsdr_read_async(). Documentation on libusb implies this as safe also.

I'd like to investigate the original problem and find a root cause and different solution. Please link or paste any info on a "Possible issue with misaligned center frequency" (perhaps in CubicSDR?) or "Issue with cheap/chained USB hubs".