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

feature: gr-iio #4277

Merged
merged 26 commits into from Jun 4, 2021
Merged

feature: gr-iio #4277

merged 26 commits into from Jun 4, 2021

Conversation

adamhorden
Copy link
Member

@adamhorden adamhorden commented Feb 23, 2021

feature: iio

This pull request is related to GNU Radio Enhancement Proposals 0017. In short it adds the gr-iio out of tree module in tree.

Please see GNU Radio Enhancement Proposals 0017:

https://github.com/gnuradio/greps/blob/master/grep-0017-iio.md

This is a rebase of the source against the original source from Travis Collins with fixes to allow GNU Radio version >= 3.9 compatibility.

Please see:

analogdevicesinc#4
analogdevicesinc#5
https://github.com/analogdevicesinc/gnuradio/tree/merge-griio-upstream
#2840

Testing

I have tested these changes by building GNU Radio version 3.9 (master) from source and applying these changes. The build works as expected. I can supply a Docker container that allows this testing to happen in an automated manor.

#33 27.63 -- Found libad9361 library: /usr/lib/libad9361.so
#33 27.63 -- Found libiio library: /usr/lib/libiio.so
#33 27.63 --
#33 27.63 -- Configuring gr-iio support...
#33 27.63 --   Dependency Boost_FOUND = TRUE
#33 27.63 --   Dependency ENABLE_GNURADIO_RUNTIME = ON
#33 27.63 --   Dependency ENABLE_GR_BLOCKS = ON
#33 27.63 --   Dependency LIBIIO_FOUND = TRUE
#33 27.63 --   Dependency LIBAD9361_FOUND = TRUE
#33 27.63 --   Enabling gr-iio support.

Co-authored-by: Edward Kigwana ekigwana@scires.com
Co-authored-by: Josh Morman jmorman@perspectalabs.com
Co-authored-by: Ryan Volz ryan.volz@gmail.com
Co-authored-by: Travis Collins travis.collins@analog.com
Signed-off-by: Adam Horden adam.horden@horden.engineering

Closes #2717.

@adamhorden adamhorden mentioned this pull request Feb 23, 2021
@jacobagilbert
Copy link
Contributor

It does not look like the QA is running for IIO.

@mbr0wn
Copy link
Member

mbr0wn commented Feb 23, 2021

Do we think there's a way we can reduce this to 1 sink and source respectively? The differences between the devices seem fairly small.

@willcode
Copy link
Member

willcode commented Feb 23, 2021

Yes, with a bunch of work. By the time you parameterize everything (e.g., number of channels), you'd be making a new driver framework. But most of that logic can go into YML. There's a lot of logic in the subclass set_params().

@mbr0wn
Copy link
Member

mbr0wn commented Mar 11, 2021

Rebased to take advantage of the new CI containers.

@mbr0wn
Copy link
Member

mbr0wn commented Mar 11, 2021

@adamhorden F33 builds are failing:

/__w/gnuradio/gnuradio/gr-iio/lib/fmcomms2_source_impl.cc: In member function 'virtual void gr::iio::fmcomms2_source_impl::set_params(long long unsigned int, long unsigned int, long unsigned int, bool, bool, bool, const char*, double, const char*, double, const char*, const char*, const char*, float, float)':
/__w/gnuradio/gnuradio/gr-iio/lib/fmcomms2_source_impl.cc:320:15: error: 'ad9361_set_bb_rate_custom_filter_manual' was not declared in this scope
  320 |         ret = ad9361_set_bb_rate_custom_filter_manual(
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[2]: *** [gr-iio/lib/CMakeFiles/gnuradio-iio.dir/build.make:173: gr-iio/lib/CMakeFiles/gnuradio-iio.dir/fmcomms2_source_impl.cc.o] Error 1
[ 53%] Building CXX object gr-iio/lib/CMakeFiles/gnuradio-iio.dir/fmcomms5_sink_impl.cc.o
/__w/gnuradio/gnuradio/gr-iio/lib/fmcomms5_sink_impl.cc: In static member function 'static void gr::iio::fmcomms5_sink_impl::set_params(iio_device*, long long unsigned int, long unsigned int, long unsigned int, const char*, double, double, const char*, const char*, float, float)':
/__w/gnuradio/gnuradio/gr-iio/lib/fmcomms5_sink_impl.cc:286:15: error: 'ad9361_set_bb_rate_custom_filter_manual' was not declared in this scope
  286 |         ret = ad9361_set_bb_rate_custom_filter_manual(
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[2]: *** [gr-iio/lib/CMakeFiles/gnuradio-iio.dir/build.make:186: gr-iio/lib/CMakeFiles/gnuradio-iio.dir/fmcomms5_sink_impl.cc.o] Error 1
[ 53%] Building CXX object gr-iio/lib/CMakeFiles/gnuradio-iio.dir/fmcomms5_source_impl.cc.o
/__w/gnuradio/gnuradio/gr-iio/lib/fmcomms5_source_impl.cc: In static member function 'static void gr::iio::fmcomms5_source_impl::set_params(iio_device*, long long unsigned int, long unsigned int, long unsigned int, bool, bool, bool, const char*, double, const char*, double, const char*, const char*, const char*, float, float)':
/__w/gnuradio/gnuradio/gr-iio/lib/fmcomms5_source_impl.cc:334:15: error: 'ad9361_set_bb_rate_custom_filter_manual' was not declared in this scope
  334 |         ret = ad9361_set_bb_rate_custom_filter_manual(
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@adamhorden adamhorden force-pushed the feature/gr-iio branch 2 times, most recently from 15d8e90 to cfb594c Compare March 15, 2021 21:03
@adamhorden
Copy link
Member Author

adamhorden commented Mar 15, 2021

I have updated this PR with a commit that removes usage of Boost where possible and uses std 🤓.

Adam Horden

@dl1ksv
Copy link
Contributor

dl1ksv commented Mar 21, 2021

I tried to build, but it failed as libiio is located at /usr/local/libiio/ on my system.

So I added

/usr/local/libiio/lib64/
to the paths in find_library and
/usr/local/libiio/include/
to find_path

in FindIIO.cmake.
Now everything is found:

-- Found libiio library: /usr/local/libiio/lib64/libiio.so
-- 
-- Configuring gr-iio support...
--   Dependency Boost_FOUND = TRUE
--   Dependency ENABLE_GNURADIO_RUNTIME = ON
--   Dependency ENABLE_GR_BLOCKS = ON
--   Dependency LIBIIO_FOUND = TRUE
--   Enabling gr-iio support.
--   Override with -DENABLE_GR_IIO=ON/OFF

but compilation fails with

Building CXX object gr-iio/lib/CMakeFiles/gnuradio-iio.dir/attr_sink_impl.cc.o
In Datei, eingebunden von /home/schroer/gnuradiocomponents/gnuradio-volker/gr-iio/lib/attr_sink_impl.h:13,
                 von /home/schroer/gnuradiocomponents/gnuradio-volker/gr-iio/lib/attr_sink_impl.cc:14:
/home/schroer/gnuradiocomponents/gnuradio-volker/gr-iio/lib/device_source_impl.h:14:10: schwerwiegender Fehler: iio.h: Datei oder Verzeichnis nicht gefunden
   14 | #include <iio.h>
      |          ^~~~~~~

even

//Path to a file.
LIBIIO_INCLUDE_DIR:PATH=/usr/local/libiio/include

//Path to a library.
LIBIIO_LIBRARY:FILEPATH=/usr/local/libiio/lib64/libiio.so

is set in CMakeCache.txt and
$ls -l /usr/local/libiio/include/
insgesamt 76
-rw-r--r--. 1 root root 76333 11. Dez 13:00 iio.h

@mbr0wn
Copy link
Member

mbr0wn commented Mar 22, 2021

I tried to build, but it failed as libiio is located at /usr/local/libiio/ on my system.

So I added

/usr/local/libiio/lib64/
to the paths in find_library and
/usr/local/libiio/include/
to find_path

in FindIIO.cmake.

This seems like a non-standard path. Do you know how those files got there? How did you install your libiio?

If it's a non-standard path, then the appropriate "fix" would be to add that path to CMake with some DIIO_LIBRARY_PATH or something like that (I didn't look up the variable name).

@dl1ksv
Copy link
Contributor

dl1ksv commented Mar 22, 2021

Yes, I don't like to put all packages to /usr/local. I installed libiio to
/usr/local/libiio

I come across this by adding the corresponding pathes to FindIIO.cmake
Then everything will be found, but the include path is not added to the compiler options, so iio.h will not be found.

Meanwhile I found some more problems, so I'd like to make a pr, but I'm unsure how to do this in this case.

@mormj
Copy link
Contributor

mormj commented Mar 23, 2021

Meanwhile I found some more problems, so I'd like to make a pr, but I'm unsure how to do this in this case.

I think you can make a PR against the feature/gr-iio branch, and then it should show up in this PR when it gets merged

@willcode willcode added the Backport-3.9 Should be backported to 3.9 label Mar 25, 2021
@adamhorden
Copy link
Member Author

@ryanvolz thanks for the pull request 😄 .
I tested this locally and have rebased the three commits into the feature/gr-iio branch.
I have also rebased master into the feature/gr-iio branch to allow CI to check these changes 🤓 .

Adam Horden

@ryanvolz
Copy link
Contributor

ryanvolz commented Apr 1, 2021

Looks like the Fedora build fails because it finds/uses libad9361 version 0.1 and some of the gr-iio code needs version 0.2. We could and probably should add a minimum version to the CMake check, but that would require adding some more code to determine the version in the Find*.cmake modules so that the version gets set in cases other than finding the library through pkg-config.

As for Fedora in particular, is that a case of needing to update the Docker image or does Fedora not package libad9361 version 0.2?

@ryanvolz
Copy link
Contributor

ryanvolz commented Apr 1, 2021

As for Fedora in particular, is that a case of needing to update the Docker image or does Fedora not package libad9361 version 0.2?

Looks like libad9361 hasn't been updated since 2017 in Fedora, and the Fedora package is what the Docker image is using.

@adamhorden
Copy link
Member Author

As for Fedora in particular, is that a case of needing to update the Docker image or does Fedora not package libad9361 version 0.2?

I am just working on a pull request for the Docker container for Fedora to bring libad9361 to version 0.2 🤓 .
For CentOS I bumped the version to version 0.2 by compiling from source. It is the same fix here for Fedora.

Once this pull request is merged my thinking is that we can add a check to determine the version in the Find*.cmake modules. CI would be at a minimal of version 0.2 and we can then enforce this version.

Adam Horden

@mormj
Copy link
Contributor

mormj commented Apr 12, 2021

@adamhorden - is the docker change for F33 ready to go?

@MountainLogic
Copy link

MountainLogic commented May 6, 2021

@adamhorden , Thanks so much for your work on iio. Its a big deal for me. I have pulled this branch of gnuradio and it is building fine. I am seeing qa_iio fail:

scott@scott:~/wip/gnuradio/build$ ctest -R qa_iio -V
UpdateCTestConfiguration from :/home/scott/wip/gnuradio/build/DartConfiguration.tcl
UpdateCTestConfiguration from :/home/scott/wip/gnuradio/build/DartConfiguration.tcl
Test project /home/scott/wip/gnuradio/build
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 222
Start 222: qa_iio

222: Test command: /usr/bin/sh "/home/scott/wip/gnuradio/build/gr-iio/python/iio/qa_iio_test.sh"
222: Test timeout computed to be: 10000000
222: Traceback (most recent call last):
222: File "/home/scott/wip/gnuradio/gr-iio/python/iio/init.py", line 31, in
222: from .iio_python import *
222: ModuleNotFoundError: No module named 'gnuradio.iio.iio_python'
222:
222: During handling of the above exception, another exception occurred:
222:
222: Traceback (most recent call last):
222: File "/home/scott/wip/gnuradio/gr-iio/python/iio/qa_iio.py", line 12, in
222: from gnuradio import gr, gr_unittest, iio, blocks
222: File "/home/scott/wip/gnuradio/gr-iio/python/iio/init.py", line 35, in
222: from .iio_python import *
222: ImportError: generic_type: type "device_source" referenced unknown base type "gr::sync_block"
1/1 Test #222: qa_iio ...........................***Failed 0.29 sec

0% tests passed, 1 tests failed out of 1

Total Test time (real) = 0.32 sec

The following tests FAILED:
222 - qa_iio (Failed)
Errors while running CTest

@adamhorden
Copy link
Member Author

@adamhorden , Thanks so much for your work on iio. Its a big deal for me. I have pulled this branch of gnuradio and it is building fine. I am seeing qa_iio fail:

Thanks. It has been allot of work to get to this stage 🤓 .

This is failing on tests. I will take a look into this. Thanks for the report. For now:

-DENABLE_TESTING=OFF 

You can disable the tests while I look into this, this branch has been tested using a ADLAM Pluto.

Adam Horden

@adamhorden adamhorden force-pushed the feature/gr-iio branch 4 times, most recently from 664713f to 131f32c Compare May 24, 2021 18:41
mormj and others added 12 commits June 1, 2021 23:55
rather than complex constructors and a catch all set_params, construct
with the minimum amount of parameters and then have them individually
settable

Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
Advanced filter configurations are now possible through the Pluto and
FMComms2 sink blocks. This includes filter designer controls and support
for the DEC8/INT8 filters now able in 2019-R2 releases from ADI. Pluto's
DEC8/INT8 filters have exists since very early firmware images.

This will also fix errors related to setting the sample rate like
"Unable to set out_voltage_sampling_frequency" since this update
properly sets the filters on sinks.

Signed-off-by: Travis F. Collins <travis.collins@analog.com>
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
mormj and others added 3 commits June 2, 2021 00:01
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
Run clang-format.

```
find . -iname '*.cc' -o -iname '*.h' | xargs clang-format -i
```

Signed-off-by: Adam Horden <adam.horden@horden.engineering>
@adamhorden
Copy link
Member Author

adamhorden commented Jun 2, 2021

I have done some refactoring and removed parameter required_enable 🤓 .

See commit hash:

7409774

This was unused and during testing was set to required_enable == false.

This code path has never been used and or tested. We have concluded after investigating and testing, this could safely be removed to enable a cleaner API.

The original code from Analog devices had this comment:

* \param required_enable  Boolean when True if an extra register_access
*        attribute write is required for use a register. This is required
*        for MathWorks generated IP.

Adam Horden

adamhorden and others added 3 commits June 2, 2021 09:27
Refactoring.

Remove parameter `required_enable`.

This was unused and during testing was set to:

`required_enable == false`

This code path has never been used and or tested. We have concluded
after investigating and testing, this could safely be removed
to enable a cleaner API.

Signed-off-by: Adam Horden <adam.horden@horden.engineering>
gr-iio depends on libiio (and not on IIO) and conditionally from libad9361.

Signed-off-by: Volker Schroer <3470424+dl1ksv@users.noreply.github.com>
Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
@mormj
Copy link
Contributor

mormj commented Jun 3, 2021

@adamhorden - in your view is this close enough to merge onto master and work from there, or do you have a list of items that need to be addressed before you are ready?

@adamhorden
Copy link
Member Author

@adamhorden - in your view is this close enough to merge onto master and work from there, or do you have a list of items that need to be addressed before you are ready?

I feel this is ready to go 😁 . I have done enough testing with actual hardware and I feel it is now at the stage where this should be merged in and anything else is a follow up PR. Same way Soapy has been merged in and following work is done as a PR.

I was just looking at the examples and seeing what could be removed this evening as some just do not work. Apart from the examples I am ready for this to be merged in 🤓 .

Adam Horden

@adamhorden
Copy link
Member Author

adamhorden commented Jun 3, 2021

@mormj I feel this is now ready to be merged 🤓 .

See commit hash:

7b25b39

This removes the examples.

Adam Horden

Remove non working examples.

Signed-off-by: Adam Horden <adam.horden@horden.engineering>
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.

Upstream gr-iio
9 participants