pothosware / SoapySDRPlay3

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

Additional optional sample rates in SoapySDRPlay driver #16

Open fventuri opened 3 years ago

fventuri commented 3 years ago

@SDRplay Andy, I am creating this new issue in the SoapySDRPlay driver to ask your opinion about the following question.

Franco Spinelli IW2DHW (@frspin) and I have been working for some time to get John Melton's G0ORX linphsdr program to work with the SDRplay RSPs using the SoapySDRPlay driver (see my pull request to Johns' repository here: https://github.com/g0orx/linhpsdr/pull/84) John's program uses a DSP library called WDSP (https://github.com/g0orx/wdsp) by Warren Pratt NR0V, that has been specifically designed for the HPSDR class of SDRs; these SDRs work with sample rates in the 48kHz 'family', i.e. 48kHz, 96kHz, 192kHz, and 384kHz.

Based on the discussions with you and Vincent a while ago (and based on what SDRuno presents to the user), I was under the impression that the RSPs would work best with output sample rates of 2MHz (and 1M, 500k, 250k, 125k, 62.5k using decimation) with an IF of 1620kHz, and with sample rates > 2MHz with a zero IF.

What we would be interested instead is to be able to use them with a sample rate of say 1536kHz and a zero IF (which with decimation would yield all the sample rates above, since 1536/4=384, 1536/8=192, and so on).

Franco (@frspin) modified his own copy of the SoapySDRPlay driver, ran same tests, and it looks like this configuration would work, but, not being familiar at all with the internals of the RSPs and the libsdrplay_api library, I wanted to ask your counsel before moving forward.

If you think these sample rate values are acceptable and do not cause other problems, I thought they could be added to the SoapySDRPlay driver, either in the main code, or perhaps using a 'cmake' compilation flag (like we used to to have for the RF gain thing a while ago), or perhaps in a different repo branch (which might make things a little more complicated since the two branches would have to be kept in sync after that). Another option would be for us to create a fork somewhere with these additional optional values of the sample rate, but I am afraid it could cause unnecessary confusion among the customers.

Thanks, Franco

SDRplay commented 3 years ago

Hi Franco,

Franco Spinelli logged a support ticket with this issue and you can get to lower arbitrary sample rates, but only in Zero-IF mode because you need a starting sample rate of between 2 and 10 MHz

So 384 kHz = 3.072 MHz with a decimation of 8 and 768 kHz would be 3.072 MHz with a decimation of 4 etc.

Obviously the only issue to be aware of with Zero-IF is the DC offset correction algorithm, so you would need an offset between the LO and the VFO for signals such as AM, although for FM this wouldn't be a problem.

Hope that helps,

Best regards,

Andy

fventuri commented 3 years ago

@SDRplay Thanks Andy - that helps a lot!

The question that I'd like to discuss next is how to best implement this set (or range) of sample rates in the SoapySDRPlay driver, since I think most of the customers use it with the CubicSDR application, and I would like to avoid (or limit) changes to the way they currently see the RSP devices and their sample rates.

There are two main places in the SoapySDRPlay driver where the client application deals with sample rates (just to make sure we are all on the same page, by 'sample rates' in this discussion I am always referring to the 'output sample rates', i.e. the sample rates as seen by the client, as per our discussion with Vincent a while ago):

In light of the current behavior, I was considering a few options (@frspin feel free to add your suggestions too):

Franco

SDRplay commented 3 years ago

Hi Franco,

I think there will always need to be a consideration between what we could do vs what the SoapySDR framework was designed to support. The classic case being the gain control issue.

My understanding is that SoapySDR was really designed to support a range of predefined sample rates, hence why there is a listSampleRates function. This has presumably been modelled on the ExtIO format where the exact same thing is done.

I would personally stick with the predefined list of sample rates which is easier to control and more in keeping with the SoapySDR framework. It is true to say that 2 MHz, 1 MHz, 500 kHz should all be delivered using Low-IF, beyond that the others you mentioned will all have to be Zero-IF. You really want to keep the sample rate as low as possible for 2 reasons...

  1. CPU load - yes you could use 6.144 MHz instead of 3.072 MHz but then the decimation filter needs to be twice as high for the same final sample rate, so not only is there an extra load on dealing with the higher input sample rate, there is also more software processing to deliver the final sample rate.

  2. ADC bits - With higher sample rates, comes lower number of bits - below 6.048 MHz, the number of bits is 14, above 6.048 MHz the number of bits goes to 12, then above 8.064 MHz it's 10 and for > 9.216 MHz it's 8 bit. This was designed to make the data all fit within the USB 2.0 bandwidth limit. So 3.072 MHz is 14 bits, but 6.144 MHz is 12 bits just to decimate down to the same final sample rate. Yes, I know that effectively every divide by 4 in the decimation filter gets you 1 bit back, but decimating 6.144 MHz down to 384 kHz only gets you back to 14 bits, which is where 3.072 MHz starts at. I don't want to get into the whole number of bits debate because at HF other factors dominate anyway, but that's for a different discussion. But you see what I mean in general.

As an algorithm, you can start with the final sample rate and keep multiplying by 2 until you get equal to or above 2 MHz and then that becomes your input sample rate.

Does that stack up with what you want to do?

Best regards,

Andy

vsonnier commented 3 years ago

Hi @SDRplay, hi @fventuri !

Just my 2 cents here on CubicSDR behaviour:

So basically a user can input any sample rate he wants.

Another small point, while I agree with @SDRplay for the others:

  1. CPU load - yes you could use 6.144 MHz instead of 3.072 MHz but then the decimation filter needs to be twice as high for the same final sample rate, so not only is there an extra load on dealing with the higher input sample rate, there is also more software processing to deliver the final sample rate.

IMHO, that part is quite small compared to the other DSP processing done later by any SDR application, we should not limit the bandwith if it is beneficial.

About the ADC bit part, I agree. Personnaly I stick to 8MHz all the time with my RSP2 because if gives the best apparent results: In particular, the other lower sample rates give worse images an dirtier signals, so I keep using 8MHz whatever the RF band I'm looking at.

SDRplay commented 3 years ago

And therein lays the problem. What I see as a developer a lot of the time, is that each of us has an experience which is different to the other and sometimes that can "bleed" into the development. In truth the API can deal with any scenario you throw at it, the reason I prefer to use low sample rates is because I'm typically using SBCs where every bit of processing power is precious. Whilst Vincent uses 8 MHz sample rate because that suits his setup.

Neither of those scenarios have really anything to do with the hardware or the API, it's either the host we choose to use or the environment (atmosphere, antenna, cable, interference, etc.) that mostly dominate.

I don't really have a preference as to how the sample rate is delivered, if the SoapySDR/CubicSDR environment can cope for arbitary sample rates, then the API can deal with that if you choose to implement it.

Just as a word of caution, when we started SDRuno, we gave the user the ability to tweak everything the API could do. That was a massive mistake, we have learned that people really want less options, but more optimisation for those options. What's really more important here than the sample rate, is what IF bandwidth you select. You have to avoid aliasing and make sure that the IF bandwidth is appropriate for the sample rate selected (whatever it ends up being).

Best regards,

Andy

fventuri commented 3 years ago

@SDRplay @vsonnier Thanks for the very useful and insightful comments, Andy and Vincent!

I think Andy's last comment about trying to keep things simple and give the user fewer but well tuned (and well tested) choices is very important. Another principle I tend to stick with is the principle of 'least surprise', which basically means that if something used to work in given way (for instance using a LIF for sample rates of 65.5kHz, 125kHz, etc), do not change it unless it is absolutely necessary and for very very good reasons, because if you do, it is going to confuse the user, possibly have other unforeseen consequences, and most importantly tarnish the feeling of stability in the application/library (i.e. "they keep changing things around"; how many times we hear that!)

Also I suspect that most (if not all) of the users use one of the predefined settings for the sample rate, i.e. the 'manual' setting in CubicSDR mentioned by Vincent is seldom used (if at all). Please correct me if I am wrong; I'd like to hear what are the reasons for selecting an output sample rate of say 100kHz, or 495kHz.

If these premises are correct (especially the one about 'manual' sample rates), I think the best approach to add support for 'linhpsdr' (and possibly other applications of the HPSDR family in the future) would be to just add the four sample rates: 96kHz, 192kHz, 384kHz, and 768kHz, as suggested by @frspin, and they would be implemented with a fsHz=3.072kHz, zero IF, and appropriate decimation factor. I also think that we should deal with values of requested sample rates that are <=2Mhz and not in the list of selectable values, by flagging them as invalid, return an error message to the user, and do not attempt to change the current sample rate. I am saying this because, after looking at the current code in the SoapySDRPlay driver, @frspin made me realize that if a user enters a manual sample rate of say 350kHz, the code would return an input sample rate of 6Mhz, an IF=1620kHz, and a decimation factor of 1 (i.e. the values for an output sample rate of 2MHz), which is wrong.

If we all agree on this approach, I can work on making these changes (in a new branch) over the weekend, we can run some tests with the various applications and use cases, and then make a final decision.

Franco

SDRplay commented 3 years ago

Sorry Franco,

You're right, I should have said that 2 MHz, 1 MHz, 500 kHz, 250 kHz, 125 kHz, 62.5 kHz should all be done with Low-IF, the rest will need to be Zero-IF

I've never used the arbitrary sample rate feature in CubicSDR - I didn't even know it existed or that the SoapySDR framework supported it!

Best regards,

Andy

jketterl commented 3 years ago

Well then, let me try to come at it from a different perspective. The focus on CubicSDR is not a good practise to start with, there's other applications out there where sample rates may have a completely different meaning: I'm talking about OpenWebRX, where the sample rate choice is mostly based on what band you're on and how much of the spectrum you'd like to share.

fventuri commented 3 years ago

@jketterl Thanks for the feedback Jakob!

As a developer it is very useful for me to see other people's perspectives, understand how the driver is used, and how to make it better and more useful.

The way I see it is that the SoapySDRPlay driver (and the SoapySDR framework in general) is trying to abstract the internal complexity of the low level hardware, and provide a sort of unified 'experience' and API to the various applications that use it; in my mind, I see a stack more or less like this: a low level library that talks directly to the hardware (like libsdrplay_api or librtlsdr) at the bottom; an 'intermediate' level API (like SoapySDR, or a GnuRadio OOT module) that provides a well defined and unified API (and because of being unified, many times it is a simplified abstraction of the underlying low level API), and finally a client application on top, like OpenWebRX, or CubicSDR (or for instance gqrx in the case of GnuRadio).

The important thing to notice here is that the intermediate level layer has sometime to make some trade-offs (or choices) between presenting a unified and consistent API to the client and the 'quirks' of the underlying library/hardware; case in point here is what to do with sample rates below or equal to 2MHz for the SDRplay RSPs: a request for say a sample rate of 500kHz could be implemented in two ways, with the LIF approach, or with the ZIF approach, but the setSampleRate() call does not have a way to specify that (and rightly so, because this is a high level API that shouldn't "bothered" with this kind of details), so the intermediate level driver has to manage this case in some way. This problem can also get even trickier when you try to implement a continuous range of sample rates, where say a request for a sample rate of 499.5kHz would be implemented one way (using ZIF), and a request for a sample rate of 500kHz would be implemented in a different way (using LIF) . A user playing around with the sample rates settings might change the sample rate from 500kHz to say 499.5kHz, notice that things are very different, and think your application has a bug because of that.

Also we need to consider is that another useful purpose of this intermediate level is to simplify things to the client application (and therefore to the end user, since many client applications like CubicSDR present that list of sample rates directly to the user) in a way that the user does not have to "think too much" about what to select (like Andy pointed out in his comments about the evolution of the UX in SDRuno); presenting a well defined set of sample rate values all implemented in a well defined way could be more useful than returning a range of [min,max] and hoping for the best.

Finally I agree 100% regarding printing an error message and returning an error for invalid sample rates; the problem in this case is that the Soapy driver API defines setSampleRate() as a function returning a void (see: https://github.com/pothosware/SoapySDR/blob/926c86d98b3a1d0b948ef6252982ee55230f1f0c/include/SoapySDR/Device.hpp#L873), so the developer is left to scratch their head on how to handle errors inside setSampleRate(): throw an exception? call abort()? (just kidding), perhaps there is a better way?

This all said, I am not at all against adding other possible sample rates below 2MHz, if we know that they are useful and used in one of the many applications that use this driver; I think we can just come up with a list, explicitly state how to uniquely implement them (LIF for 62.5, 125, 250, etc; ZIF for the others), and I'll be more than happy to change the code to do that (of course the list of sample rates is not cast in stone; it could be modified in the future as we see fit).

Franco

jketterl commented 3 years ago

The best approach, from my experience, is to just provide both, i.e. an API that provides reasonable defaults so you can get usable results in a very quick fashion, but then also expose the advanced settings for those that know what they're doing.

Lower sample rates do make sense for OpenWebRX, especially on shortwave, where there is some very narrow HAM bands (the narrowest is 30m I believe, being only 50kHz wide). Some users have experienced aliasing issues with the lower sample rates, though (https://groups.io/g/openwebrx/topic/strange_behavior_with_rsp1/78060989). I haven't had much time to experiment with it, and maybe there is a way around this, but this suggests that there's a lower limit somewhere.

The high sample rates, on the other hand, are useful for spanning multiple bands at the same time. The main use case for this is spotting / skimming applications, or maybe from 70cm upwards. Both require some CPU grunt, though, of course.

As for the error handling: I did not know that the C++ API specifies a void return type, the correspondig C API that I've been using returns an int. That conversion seems to be done by catching exceptions, so I suppose that's probably the best way to go.

See: https://github.com/pothosware/SoapySDR/blob/master/lib/DeviceC.cpp#L562 https://github.com/pothosware/SoapySDR/blob/master/lib/ErrorHelpers.hpp#L13

fventuri commented 3 years ago

@jketterl Thanks Jakob for your feedback.

On the high sample rates (i.e. above 2MHz) the current approach of fsHz=<sample rate>, Zero IF, and decimation factor=1 should work just fine, and should allow (at least in the call of the setSampleRate() function) to set any value you want.

It's with the lower sample rates (<= 2MHz) that things get tricky. In the specific example you mention of the 30m band, with a 50kHz bandwidth, would your users be OK with an I/Q (quadrature) sample rate of 62.5kHz, for instance, instead of exactly 50kHz? The SDRplay RSPs also have a set of bandwidth filters, but the narrowest one is 200kHz (if I remember correctly), which means that when you go down to those very low sample rates, there is so much that the RSP hardware can do for you, and the user may need additional external filtering to avoid aliases.

Another factor to consider, which I didn't mention in my previous comment, is that the RSPduo hardware in dual tuner or master/slave configuration has to work in LIF mode, i.e for that specific hardware in that specific configuration, the only sample rates possible are 62.5kHz, 125kHz, 250kHz, 500kHz, 1MHz, and 2MHz, and that's it.

I plan to make the changes for the additional sample rates over the weekend, and put them in a different branch; after that, we can run some tests with different clients, different hardware, and different configurations, see how it goes, and see how this driver can be improved.

Also if you know of other applications using this driver, I think it would be a good time to invite their developers to join this discussion, so that we can reach a large consensus on this subject.

Franco

vsonnier commented 3 years ago

Hello all, I've added a commit to CubicSDR : https://github.com/cjcliffe/CubicSDR/commit/7b1956f7cd48e9adb520d862814573dd3130e59f

This commit attempts to better manage the case when the user request a setSampleRate incompatible with the hardware.

  1. Catching exception on setSampleRate : this is really bad, please do not do this, though.
  2. Irrespective of 1., always re-read the actual sample rate by getSampleRate and register it as the effective user setting.

Throwing execptions is bad because nothing in this API suggests that a call may fail in such brutal way. We cannot reasonnably force SoapySDR applications to wrap every API call into try-catch blocks and manage it that way. IMHO, this API must be exception-free.

On the other hand, for each set there is always a get where a client application can read the current effective settings, as an option.

All the discrete sample rates of the RSP that cannot be applied exactly should indeed be implemented that way:

jketterl commented 3 years ago

Throwing execptions is bad because nothing in this API suggests that a call may fail in such brutal way. We cannot reasonnably force SoapySDR applications to wrap every API call into try-catch blocks and manage it that way. IMHO, this API must be exception-free.

How can you expect to just call a method and not perform any kind of error handling?

Since the API does not allow passing a result as a return value, exceptions are the only thing that I can think of on how to handle any kind of error. This is done in SoapySDR itself, too, see: here or here.

I'm not aware of any guiding documentation for implementing device modules, if there's any it would certainly be helpful. So far I'm only reading the code, and there's multiple indications that exceptions will be and should be thrown. That's at the level of SoapySDR, so deviating here is a bad idea.

As for your suggestion about guessing the sample rate: It would be a lot of work to implement it this way for me, and would still be confusing. As stated before, a corresponding error would work better for my application.

vsonnier commented 3 years ago

Since the API does not allow passing a result as a return value, exceptions are the only thing that I can think of on how to handle any kind of error. That's at the level of SoapySDR, so deviating here is a bad idea.

This is not in the contract or in nowhere the general documentation. This is even worse than a return value, because we don't know the type of exceptions supposed to be thrown. And no, std::runtime_error is not SoapySDR specific and convey no useful information.

This is done in SoapySDR itself, too, see: here or here.

Those examples are not that good. They cover device initialization and deinit, which may not fail except in rare circumstances.

On the other hand, thowing exceptions all over the place while the device has actually been initialized properly, while the application has previously configured the driver according to its advised capabilities (valid sample rates in particular), is certainly not an expected runtime beahaviour, IMHO.

As for your suggestion about guessing the sample rate: It would be a lot of work to implement it this way for me,

This is what the RSP driver does already choosing internal settings according to the requested setSampleRate. (decimation, Zero-If vs. Low-IF...etc.) and this is what make this driver complicated in the first place. Now choosing settings for an arbitrary sample rate < 2MHz is maybe too complicated indeed.

What I'm proposing is to stick to predefined discrete sample rates < 2MHz by choosing the closest value with pre-known settings hard-coded to the driver. This is even the only possibility for Master-Slave configs when only some sample rates may be valid at all.

At the end of the day @fventuri is the one doing the real work here, so choosing the right solution is up to him.

My personnal conclusion is, as far as i can see of previous coments that handling an arbitrary sample rate < 2MHz is too difficult so for simplification sake should be rejected if not in pre-defined discrete values either :

jketterl commented 3 years ago

This is not in the contract or in nowhere the general documentation.

I don't know the contract, nor where to find it. If somebody has it, please post it. If there's no contract, I suppose it's best to ask the guys at SoapySDR. After all, this is about implementing an API rather than defining it.

std::runtime_error is not SoapySDR specific and convey no useful information.

I agree. I was kind of confused when I saw the runtime exception there.

Those examples are not that good. They cover device initialization and deinit, which may not fail except in rare circumstances.

That's just code I have had a look at while working with SoapySDR. I can search for more, if you wish. Stating that device initialization may not fail however is very optimistic.

fventuri commented 3 years ago

Given all the ideas and suggestions, I thought it would be useful to proceed a step at a time, so for now I just implemented only those four additional sample rates (96kHz, 192kHz, 384kHz, 768kHz), and limited the possible sample values <=2Mhz to only the set of the previous Low IF values (62.5kHz, 125kHz, 250kHz, 500kHz, 1MHz, and 2MHz) plus the additional values. If the selected sample rate value is <=2Mhz, and is not one of the ones listed above, setSsampleRate() will print a 'WARNING' message, leave the current sample value unchanged, and simply return.

I decided for this approach (for now) for a couple of reasons:

As I said earlier, I am open to ideas and suggestions; I'd also like to hear Andy's @SDRplay opinion when he is back at the office on Monday.

The branch with these changes is called 'extra_sample_rates' (https://github.com/SDRplay/SoapySDRPlay/tree/extra_sample_rates); I quickly tried it with the latest version of CubicSDR compiled from the current master, and it didn't crash, so that's a good sign :-)

Also Franco @frspin found a bug with a case of streaming buffer overflow, where in same cases the buffer was getting resized to a value larger than its capacity, and therefore causing an invalid pointer and then a crash; this is now fixed, both in master and in the 'extra_sample_rates' branch.

Give it a try when you have time, and let me know how it goes.

Franco

vsonnier commented 3 years ago

@jketterl:

I don't know the contract, nor where to find it.

To be honest what I call 'contract' here is just the code coments in the API source code: https://github.com/pothosware/SoapySDR/wiki#client-api, I don't think there are other requirements.

As you can see there is even a C-wrapper API where setSampleRate has a return value... So you have a point making the C++ API throwing exceptions if we want an equivalent functionnality.

Too bad, because there is no standard SoapySDR exception so we a are stuck throwing std::runtime_error or similar in this case, and hoping the client application manage it.

I've done it for Cubic, but I still don't like it : I used a dirty catch(...) around the call to setSampleRate for portability, with no way of properly interpreting that exception.

fventuri commented 3 years ago

Quick check with this group on the testing of these changes. Franco (@frspin) and John (@g0orx) both reported that the additional sample rates work with linhpsdr.

Unless I hear any objections, I plan to merge these changes into master tomorrow evening.

Also, if you are interested, we can open a new thread to discuss a proper way to handle errors in the SoapySDRPlay driver.

Franco

vsonnier commented 3 years ago

Thanks @fventuri, I didn't had time to test myself, but not changing the sample rate in case of unsupported value is probably the way to go.

Speaking of error handling, look at that comment @jketterl and all, by PothosWare author himself: https://github.com/pothosware/SoapyRTLSDR/issues/50#issuecomment-718028223

So, hem... I'll be damned apparently throwing exceptions is considered cool to report errors in a SoapySDR module. Here in this particular fix std::runtime_error was used : https://github.com/pothosware/SoapyRTLSDR/commit/52cb5c0ab595adcb50262baacc6eaf9e83ae4793 as you can see.

fventuri commented 3 years ago

@vsonnier, since the topic of this issue (adding a few extra sample rates) and the right way of handling the errors are (almost) orthogonal, I suggest that first I complete this change by merging it tomorrow.

After that I'd like to open a new issue about the error handling; there we can continue this discussion (if further discussion is needed), and I'll create a new branch with the code changes needed to implement throwing a std::runtime_error exception when setSampleRate() has a problem (invalid sample rate or other).

Franco

vsonnier commented 3 years ago

I'll create a new branch with the code changes needed to implement throwing a std::runtime_error exception when setSampleRate() has a problem (invalid sample rate or other).

My previous remarks still stands though : I've not found (maybe yet) recomendations or would be-specs by @guruofquality on which exceptions are advised or not to throw, so the generic handling could only be by black-hole catch (...) blocks on the client application side. Sigh.

So if you decide to go the exception route, you'll have to care to make the getXXX calls consistent (reporting the currently applicable values) so that a client can interrogate the module after catching exceptions to manage the situation properly.

Thanks for your great work !

fventuri commented 3 years ago

I merged these new extra sample rates into master.

I also created a new issue here: https://github.com/SDRplay/SoapySDRPlay/issues/19 to discuss how to best handle errors due to invalid selected values and/or bad return codes.

I am going to leave this issue open for another few days in case there is a problem due to the additional sample rates; otherwise I would suggest that we continue the discussion about error handling in that thread.

Franco