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

Add support for BladeRF 2.0 Micro. #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

elafargue
Copy link

dump1090 also accepts a new --bladerf-biastee switch to enable the Micro's bias Tee on RX ports.

Did not test that BladeRF 1 was still working, but it should be, if anyone
has a BladeRF 1 to test with, please report.

--bladerf-biastee switch to enable the Micro's bias Tee on RX ports.

Did not test that BladeRF 1 was still working, but it should be, if anyone
has a BladeRF 1 to test with, please report.
@mutability
Copy link

Does this build on stretch? The bladerf_frequency API changes have been a problem there in the past.

@elafargue
Copy link
Author

Yes, it builds on Stretch, but with libbladerf2 - that one is not in stretch by default, but available on the Debian package repository. I suggest to deactivate compiling support for BladeRF by default on the rules file for Debian

@mutability
Copy link

There is a strong requirement that we have bladeRF support enabled by default on both jessie and stretch, as FlightAware has many remotely deployed bladeRFs where the host runs jessie/stretch.

Building and distributing updated bladerf packages for stretch is a possibility (we already do that for jessie), but I'd much rather just have any changes be version-aware and handle building against older bladerf out of the box.

@elafargue
Copy link
Author

Unless I am mistaken, I'm afraid libbladerf1 only supports the older BladeRF boards, is that correct? So if we want to support all models, libbladerf2 is really the only option, which I agree is a pain...

@mutability
Copy link

mutability commented Apr 16, 2019

Have the build look at the headers to work out what bladerf library is installed (there are some versions defines, I think?) and build accordingly. (I mean preprocessor directives in the source)

@elafargue
Copy link
Author

That should be possible - the API has changed a lot between the versions though, so it won't be super pretty :)

@ztmr
Copy link

ztmr commented Apr 16, 2019

The master does not build at all because of the frequency datatype mismatch. I've quickly hacked it here but this PR seems to be much more sophisticated approach so I am not even going to raise my own.

Is there any reason why this cannot be merged straight away?

@mutability
Copy link

Is there any reason why this cannot be merged straight away?

That change breaks builds on stretch, which is the primary supported version.
What OS, architecture, and version do you have problems building on?

@ztmr
Copy link

ztmr commented Apr 17, 2019

This is the OS version where I was building from scratch (everything including bladeRF):

$ lsb_release -a
LSB Version:	core-9.20160110ubuntu0.2-amd64:core-9.20160110ubuntu0.2-noarch:security-9.20160110ubuntu0.2-amd64:security-9.20160110ubuntu0.2-noarch
Distributor ID:	Ubuntu
Description:	Ubuntu 16.04.6 LTS
Release:	16.04
Codename:	xenial
$ uname -m
x86_64
$ 

And this is the system where I have tried to build it using the install script from jprochazka/adsb-receiver:

$ lsb_release -a
No LSB modules are available.
Distributor ID:	Raspbian
Description:	Raspbian GNU/Linux 9.8 (stretch)
Release:	9.8
Codename:	stretch
$ uname -m
armv7l
$ 

I believe it is a best practice to use the data type exposed by the library instead of hardcoding the currently used underlying type. Changing unsigned to bladerf_frequency should not be breaking anything on any platform as it is just an alias for uint64_t (in the latest bladeRF version).
It might be that some versions of some compilers, even with -Wall set, did this conversion implicitly, not sure.

The only reason I am commenting here is because it looks like this PR is superset of my change. If it does not work as is, do you want me to raise another PR just for that one small change?

@mutability
Copy link

So you're using a custom build of libbladerf, not the OS-provided version?

Changing unsigned to bladerf_frequency should not be breaking anything on any platform as it is just an alias for uint64_t (in the latest bladeRF version).

It breaks builds when the OS-provided version of libbladerf does not provide bladerf_frequency at all. You need to make it conditional on the libbladerf version.

@mutability
Copy link

Description: Ubuntu 16.04.6 LTS
Release: 16.04
Codename: xenial

Current dev branch builds with no changes for me on xenial, using the xenial-supplied libbladerf (0.2016.01~rc1-3)

@ztmr
Copy link

ztmr commented Apr 17, 2019

OK, I see. There are two precompiled packages, one for 1.x and another for 2.x.
What about something like this then?
It seems to work well: https://gitlab.com/ztmr/dump1090/pipelines/57288072

@mutability
Copy link

What is source of the second precompiled package? I don't see a libbladeRF2 in anything other than Debian experimental (and if you're using experimental then you're pretty much on your own..)

The change looks OK except you probably need to narrow down the API version (the type change dates back to at least https://github.com/Nuand/bladeRF/blob/a0d498e7232c6e856e1fbb9cf1a037a7bddd37c9/host/libraries/libbladeRF/include/libbladeRF.h which has an api version of 0x01090200)

@ztmr
Copy link

ztmr commented Apr 19, 2019

OK, I have misunderstood -- they say the frequency internal data type has changed since 2.x.x but the abstract type has first appeared at 1.9.x. So the current code works but is not clean. Let me change it.

The 2.x version of the library is taken from the PPA repositories published by Nuand: https://launchpad.net/~bladerf -- which is Ubuntu only, unfortunately. Does not mean the package cannot be installed but the repos are not compatible.

Do you have your own CI/CD or you want me to keep the GitLab CI integration bit included with the PR?

@ztmr
Copy link

ztmr commented Apr 19, 2019

So just git-blamed the header and it looks like the type has appeared in 1.9.2 for the first time. But according to tags, 1.9.2 was not released and since the type is missing in 1.9.1 and is defined in 2.0.0, I would say 1.9.2 was just pre-2.0.0 changes. Are we ok to leave the conditional for >=2.0.0?

@mutability
Copy link

I committed a minimal polyfill for building on both bladerf1 & bladerf2 in e419719

@mutability
Copy link

For the original PR, I'd lean towards a similar polyfill-like approach, i.e. assume that we're on libbladeRF2 and provide implementations of the missing functions if we're actually on the older API. Then at least we only need the one conditional block not ifdefs scattered throughout the code.

@ztmr
Copy link

ztmr commented Apr 28, 2019

Yup I was thinking of polyfill for exactly the same reason (not having ifdefs all over) but came to conclusion that for just one occurence, it would be more confusing than useful from readability perspective.
Might be cleaner to define a new type bladerf_frequency_poly that clearly indicates that it is not the original type but only sort of proxy that might be either a pass-through to the native type or a custom polyfill. I know we are talking about one line of code for a while now but as you noted -- the original PR would use this pattern more than once for sure.

@mutability
Copy link

Now that I've simplified the SDR interface, I'm leaning a bit more towards just having this as a separate SDR implementation (i.e. sdr_bladerf2.c) and build one or the other depending on what's available. I don't have the resources to retest the FPGA changes used with v1 (+ needed on the FlightFeeders that we support that have bladerf boards) right now so we could tear those changes out of the v2 path to simplify it further.

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