Merge changes from Leif / rtlsdr_set_tuner_gain()

[Alexander Kurpiers]

Hi,

in the course of the development of our noise figure measurement tool
(CANFI http://www.canfi.eu) we tried to use librtlsdr with E4k dongles.
Unfortunately it turned out that the original library is too "deaf" -
the noise figure is too high due to not enough gain.

After some discussions I found that Leif already had solved this problem
and provides a patched version of the librtlsdr code (latest version
http://www.sm5bsz.com/linuxdsp/hware/rtlsdr/rtl-sdr-linrad4.tbz).

Essentially, the main contribution I needed was the additional gain
modes discussed here
http://lists.osmocom.org/pipermail/osmocom-sdr/2012-October/000296.html

I used the /E4K_GAIN_MODE_SENSITIVITY

/As I did not want to use a "foreign" library (mainly due to the fact
that providing a Windows DLL is some work...), I had a look at what
would be needed to address the issues mentioned here
http://lists.osmocom.org/pipermail/osmocom-sdr/2012-October/000297.html
and made a number of patches to merge Leif's changes back into the
"normal" library.

While at it I also merged other (unrelated) changes I found in Leif's code.

Recently I created a fork for this
(https://github.com/dl8aau/librtlsdr), but can of course also provide
separate patches.


(side note: the above fixes the initial issue by using
rtlsdr_set_tuner_gain() to also set the IF gain - the original function
for E4000 only dealt with LNA and mixer gain.
But the implementation of this function is not the same for the
different tuners anyway:
rtlsdr_set_tuner_gain() sets mixer gain by a fixed algorithm together
with LNA gain (E4K and R820T). For E4K only there is an additional
function rtlsdr_set_tuner_if_gain() - but the caller needs to know the
"details" of the IF gain stages. For other tuners there is no way to
change the IF gain currently - rtlsdr_set_tuner_gain() sets it to to a
fixed value on R820T. FC0013 only sets LNA gain for
rtlsdr_set_tuner_gain(), FC2580 does nothing and for FC0012 I'm not clear.

So currently, the function of rtlsdr_set_tuner_gain() is not consistent
at all and rtlsdr_set_tuner_if_gain() is only supported on E4000.

It was ok for the CANFI application on E4K, as I could set LNA, mixer
and IF gain through rtlsdr_set_tuner_gain() and
rtlsdr_set_tuner_if_gain(), but it was more "by chance"...

So maybe it would make more sense to implement the
rtlsdr_set_tuner_gain() the way Leif did and set suitable combinations
for LNA, mixer and IF gain for two modes: good sensitivity and good
linearity. Do this not only for E4K, but also for the other tuners that
allow for manual gain control.
Additionally, support setting LNA, mixer and IF gain separately.
IMHO that would be cleaner.
At least for R820T I need a way to control the LNA, mixer and IF gain -
or use an optimized table of gain settings.
)


Regards,

Alexander

[Alexander Kurpiers]

[Alexander Kurpiers]

[Alexander Kurpiers]

[Alexander Kurpiers]

[Alexander Kurpiers]

[Alexander Kurpiers]

[Pete Zaitcev]

On Fri, 05 Sep 2014 10:39:28 +0200

@@ -1707,9 +1709,14 @@ static int _rtlsdr_alloc_async_buffers(rtlsdr_dev_t *dev)
if (!dev->xfer_buf) {
dev->xfer_buf = malloc(dev->xfer_buf_num *
sizeof(unsigned char *));
+ if (dev->xfer_buf == NULL)
+ return -1;
- for(i = 0; i < dev->xfer_buf_num; ++i)
+ for(i = 0; i < dev->xfer_buf_num; ++i){
dev->xfer_buf[i] = malloc(dev->xfer_buf_len);
+ if (dev->xfer_buf[i] == NULL)
+ return -1;
+ }
}

Please don't do stuff like that. And I'm not just talking about
the wrong indentation. If a function runs a bunch of allocs inside,
and one of them fails, it must unroll those that succeeded before
returning the error to the caller. Failure to do it is why you

- _rtlsdr_alloc_async_buffers(dev);
+ /* Check the error code. Free return if errors */
+ if(_rtlsdr_alloc_async_buffers(dev) < 0) {
+ _rtlsdr_free_async_buffers(dev);
+ return -1;
+ }

Just say no to programming like this. The object is either initialized
or not initialized, and never half-way initialized.

-- Pete

[Alexander Kurpiers]

Hi Pete, thanks for the review.

Post by Pete Zaitcev
On Fri, 05 Sep 2014 10:39:28 +0200

@@ -1707,9 +1709,14 @@ static int _rtlsdr_alloc_async_buffers(rtlsdr_dev_t *dev)
if (!dev->xfer_buf) {
dev->xfer_buf = malloc(dev->xfer_buf_num *
sizeof(unsigned char *));
+ if (dev->xfer_buf == NULL)
+ return -1;
- for(i = 0; i < dev->xfer_buf_num; ++i)
+ for(i = 0; i < dev->xfer_buf_num; ++i){
dev->xfer_buf[i] = malloc(dev->xfer_buf_len);
+ if (dev->xfer_buf[i] == NULL)
+ return -1;
+ }
}

Please don't do stuff like that. And I'm not just talking about
the wrong indentation. If a function runs a bunch of allocs inside,
and one of them fails, it must unroll those that succeeded before
returning the error to the caller. Failure to do it is why you

True - I only ported the original code from Leif without paying
attention (I only worked myself on 0002-0005).
Sorry, I should have noticed.

Post by Pete Zaitcev

- _rtlsdr_alloc_async_buffers(dev);
+ /* Check the error code. Free return if errors */
+ if(_rtlsdr_alloc_async_buffers(dev) < 0) {
+ _rtlsdr_free_async_buffers(dev);
+ return -1;
+ }

Just say no to programming like this. The object is either initialized
or not initialized, and never half-way initialized.

I agree. I'd say we drop the whole patch 0006 and do it properly.
I'll have a look later.

[Alexander Kurpiers]

I agree. I'd say we drop the whole patch 0006 and do it properly. I'll
have a look later.

Next try...

[Pete Zaitcev]

On Sat, 06 Sep 2014 16:06:28 +0200

Post by Alexander Kurpiers
Next try...

Looks good as far as general structure goes. Style is a little odd,
perhaps. For example, /* Check the error code. */ comment is useless,
straight from the book of "a=b; /* assign a value of b to a */".
When you unroll, the ideom is while(i--). If you hide the decrement
inside a [] inside the loop body, it's liable to get missed one day.
But I'll let the maintainer take issues with style. You got the gist
of it right, I think.

-- Pete

[Alexander Kurpiers]

Hi Pete,

Post by Pete Zaitcev
On Sat, 06 Sep 2014 16:06:28 +0200

Post by Alexander Kurpiers
Next try...

Looks good as far as general structure goes. Style is a little odd,
perhaps. For example, /* Check the error code. */ comment is useless,
straight from the book of "a=b; /* assign a value of b to a */".

True - I left that from the original code. Could indeed be removed - and
a command added instead about moving of

dev->async_status = RTLSDR_RUNNING;

which is necessary (otherwise the thread does not stop despite the
error returned...).

Post by Pete Zaitcev
When you unroll, the ideom is while(i--). If you hide the decrement
inside a [] inside the loop body, it's liable to get missed one day.

Also true, but would need to be --i>=0, as i is the index of an entry
that is already NULL. Not sure if that is nicer.
I have not checked what the libusb_free_transfer() does if called with
NULL. At least for v1.0 it seems ok
(http://libusb.sourceforge.net/api-1.0/group__asyncio.html#ga6ab8b2cff4de9091298a06b2f4b86cd6)
If that works for all variants of libusb, memset'ing after malloc (or
calloc?) to zero the pointers and calling _rtlsdr_free_async_buffers()
may be easier than unrolling (less code duplication).

Post by Pete Zaitcev
But I'll let the maintainer take issues with style. You got the gist
of it right, I think.

Thanks ;-)

Yet I would prefer a comment on patch 3 - that's the only one that I'm
really interested in.
This one is only to port all the changes between Leif's code and the
current git code.

Regards,

Alexander

[Alexander Kurpiers]