search

kit-cel / gr-radar

GNU Radio Radar Toolbox
GNU General Public License v3.0
237 stars 83 forks source link

Support for GNU Radio v3.8 #36

Closed primercuervo closed 3 years ago

primercuervo commented 4 years ago

cc: @marcusmueller @mbr0wn @stwunsch

marcusmueller commented 4 years ago

Without having looked at migration too closely, what about the generated Doxygen directory? Should that really be part of this?

primercuervo commented 4 years ago

Hey @marcusmueller, thanks a lot for having a look at this.

I assume you are referring to the docs folder? I really don't see why this shouldn't be part of this, as it conforms with the newer gr-modtool generated files. Some of the changes in those auto-generated files make the whole codebase python2/3 compatible.

Would you rather that I leave that folder untouched? or are you perhaps referring to a different set of files?

FFY00 commented 4 years ago

If they are generated most of the cases they don't belong in git. You should generate them in your build proccess.

primercuervo commented 4 years ago

@FFY00 the files I'm referring to are "gr-modtool generated", not "build-time generated".

dkozel commented 3 years ago

FYI, just built this as a test and it compiles successfully with gcc against GNU Radio 3.8.1.0, but fails when using Clang. I still think this should be merged and a maint-3.8 branch made, but it would be nice to test with Clang at some point and address the type conversion errors.

signal_generator_sync_pulse_c_impl.cc:74:41: error: implicit conversion from '_Complex int' to 'float' is not permitted in C++

ryanvolz commented 3 years ago

FYI, just built this as a test and it compiles successfully with gcc against GNU Radio 3.8.1.0, but fails when using Clang. I still think this should be merged and a maint-3.8 branch made, but it would be nice to test with Clang at some point and address the type conversion errors.

signal_generator_sync_pulse_c_impl.cc:74:41: error: implicit conversion from '_Complex int' to 'float' is not permitted in C++

I would also love to see this get merged, and then a release tagged, so I can publish a conda package on conda-forge (they require building from a release tarball).

I started on the recipe many months ago, and in fact already have a few patches to get it building on macOS and Windows. I think this is the one that would fix the problem that Derek notes:

https://github.com/ryanvolz/staged-recipes/blob/gnuradio-radar/recipes/gnuradio-radar/0003-Use-gr_complex-instead-of-1j-for-non-gcc-compatibili.patch

Then I also have these other patches: https://github.com/ryanvolz/staged-recipes/blob/gnuradio-radar/recipes/gnuradio-radar/0001-Replace-not-with-for-MSVC-compatibility.patch https://github.com/ryanvolz/staged-recipes/blob/gnuradio-radar/recipes/gnuradio-radar/0002-M_PI-GR_M_PI-to-match-gnuradio-and-have-MSVC-compati.patch https://github.com/ryanvolz/staged-recipes/blob/gnuradio-radar/recipes/gnuradio-radar/0004-Do-not-link-gnuradio-radar-with-Python-not-necessary.patch

I probably should put a PR together, but anyone can feel free to submit these if they get to it before I do.

ryanvolz commented 3 years ago

I probably should put a PR together, but anyone can feel free to submit these if they get to it before I do.

Alright, I just did the PR over at #41. It goes on top of this PR.

primercuervo commented 3 years ago

@ryanvolz thanks for adding those commits!

@marcusmueller @mbr0wn @stwunsch please let us know if there is something we can do to move this forward (probably better just to look #41). We could also start the 3.9 support (I haven't seen if someone has pushed work on that), but you'd understand that this PR being not looked hinders the efforts for further maintenance.

mbr0wn commented 3 years ago

@marcusmueller @stwunsch Also, consider if kit-cel is the right place to keep this OOT. We would be happy to move the canonical upstream of this repo to github.com/gnuradio.

marcusmueller commented 3 years ago

@primercuervo am I right to say that merging #41 made this obsolete?

primercuervo commented 3 years ago

@marcusmueller I believe it does! Thank you for checking it out! :smile: