dump1090 icon indicating copy to clipboard operation
dump1090 copied to clipboard

Add support for BladeRF 2.0 Micro.

Open elafargue opened this issue 6 years ago • 19 comments

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.

elafargue avatar Mar 31 '19 17:03 elafargue

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

mutability avatar Apr 13 '19 03:04 mutability

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

elafargue avatar Apr 15 '19 03:04 elafargue

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.

mutability avatar Apr 16 '19 04:04 mutability

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...

elafargue avatar Apr 16 '19 05:04 elafargue

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)

mutability avatar Apr 16 '19 05:04 mutability

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

elafargue avatar Apr 16 '19 15:04 elafargue

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?

ztmr avatar Apr 16 '19 18:04 ztmr

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?

mutability avatar Apr 17 '19 00:04 mutability

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?

ztmr avatar Apr 17 '19 10:04 ztmr

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 avatar Apr 17 '19 10:04 mutability

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)

mutability avatar Apr 17 '19 15:04 mutability

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

ztmr avatar Apr 17 '19 16:04 ztmr

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)

mutability avatar Apr 19 '19 05:04 mutability

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 avatar Apr 19 '19 09:04 ztmr

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?

ztmr avatar Apr 19 '19 09:04 ztmr

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

mutability avatar Apr 28 '19 07:04 mutability

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.

mutability avatar Apr 28 '19 07:04 mutability

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.

ztmr avatar Apr 28 '19 10:04 ztmr

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.

mutability avatar Aug 24 '20 06:08 mutability