Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix parameter value check for airspy_set_linearity and airspy_set_sen… #53

Closed
wants to merge 2 commits into from
Closed

Conversation

TLeconte
Copy link
Contributor

@TLeconte TLeconte commented Jan 28, 2018

fix issue #52

@touil
Copy link
Member

touil commented Feb 3, 2018

Are you aware of this?
https://github.com/airspy/airspyone_firmware/blob/master/airspy_m0/airspy_m0.c#L162
https://github.com/airspy/airspyone_firmware/blob/master/airspy_m0/airspy_m0.c#L166

This is not an RTL dongle. The IF bandwidth is already matched to the sample rate in the firmware.

@TLeconte
Copy link
Contributor Author

TLeconte commented Feb 3, 2018

hum sorry, I did not to ask for merge about this set_bandwidth soon. I just wanted to commit it on my fork .... my bad,

To answer your question , hopefully you set the If bandwidth according to the sample rate, but there are cases where man want narrower bandwidth than half the sample rate. Particularly as the airspy don't allow to much choice about sample rate.

In fact, my set_bandwidth code is just a direct port of airspy linrad set bandwidth code. I wanted to measure and improve it before asking for merging ...

@touil
Copy link
Member

touil commented Feb 3, 2018

Linrad's R820T IF code is itself based on our firmware code.

@TLeconte
Copy link
Contributor Author

TLeconte commented Feb 3, 2018

But add the possibility to change bandwidth down to 0.5Mhz
See https://sourceforge.net/p/linrad/code/HEAD/tree/trunk/airspy.c
lines : 380 , 403 and 967

@touil
Copy link
Member

touil commented Feb 3, 2018

So can SDR#, SDR-Console, etc. There's a good reasons why airspy_r820t_write() exists.
First, the IF filter is not symmetrical, which requires extra DSP processing (frequency translation + decimation) and retuning to get IF centered again. That's too application specific to be put in the library. Yet, it's a bad idea expose a half-assed API that hides these details. Either you add the accompanying DSP and retune code, OR don't hide the details. A "bad abstraction" costs more in the long term than no functionality.
Leif is a smart grandpa and knows about these details. That's why he added the app specific code in his app and not in the library.
Nothing was done by chance.

@TLeconte
Copy link
Contributor Author

TLeconte commented Feb 3, 2018

As I said, the intent was not to ask for merging this commit . I know that the filter is not symmetric.I will not call set_bandwidth a half-assed API but I agree that it needs to do the job as expected which need more works.
About Linrad, I did not see the retune part which exist in the librtlsdr (but I could miss it , I did not read all the code).
And no, I don't program by chance .

@touil
Copy link
Member

touil commented Feb 3, 2018

OK. No problem. In general we call APIs that do half the job "half assed APIs", or more politically correct, "bad abstractions". If you find a more elegant solution that satisfies the semantics of IF filtering, you are more than welcome to contribute it.

@TLeconte
Copy link
Contributor Author

TLeconte commented Feb 3, 2018

and what about my first commit that fix an erroneous bound check ?

@touil
Copy link
Member

touil commented Feb 3, 2018

Ben will check and merge. There's still a lot of left-over from HackRF.

@bvernoux bvernoux changed the title fix parameter value check for airspy_set_lineariry and airspy_set_sen… fix parameter value check for airspy_set_linearity and airspy_set_sen… Feb 4, 2018
@bvernoux
Copy link
Member

bvernoux commented Feb 17, 2018

Could you rebase your code and merge with actual master as there is conflict (mainly with new fprintf(stderr ....) see minor stuff #54

@TLeconte
Copy link
Contributor Author

ok, will redo a better pull request

@TLeconte TLeconte closed this Feb 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants