fair-acc / gr-digitizers

GNU General Public License v3.0
3 stars 3 forks source link

Gr40 port #108

Closed frankosterfeld closed 1 year ago

frankosterfeld commented 1 year ago

This is the not-yet complete port of gr-digitizers to GNU Radio 4.0. The ported code was moved to blocklib/, the unported code is still in its old locations. I sort-of successfully ran the picoscope 4000a unit test against the hardware we have at KDAB (sort of successful as in the single tests worked, but the second and later tests failed with "device not found" when I ran more than one test at a time) ps3000 and ps6000 were ported but not tested against hardware.

open issues are marked with TODO(PORT), disabled code with #ifdef PORT_DISABLED.

To avoid further regressions before we have stable base, I did only slight modernizations (C++17/20) to the code where I touched it anyway, but kept especially digitizer_block_impl and picoscope subclasses mostly as it is, to not accidentally break the logic there.

frankosterfeld commented 1 year ago

Nice !

New for me that you are using meson instead of cmake now. Fine for me ... though any specific reason to do so ?

This patch follows what they do in GNU Radio 4.0 upstream, which is switching from CMake to meson.

I am sure it would be possible to get a better diff by doing some console/git gymnastics when checking out the branch locally, though may I ask you to just keep the files in their original location instead, in order to simplify the review ? What is the opinion of the other reviewers about it ?

Keeping the old structure isn't really possible/desired with the new meson build system, their meson build makes a lot of assumptions and introduces new conventions on the organisation in the file system, which includes the new blocklib/digitizers/... structure. Besides that, the class structure and hierarchy changed also (with the interface header generated by meson/GR from the .yml files, and our code being in the new foo_cpu.h/cc files), so even if the rough structure could be maintained, the files would still be renamed and change a lot.

I see how that makes code review tricky, but I don't know how to fix that...

alexxcons commented 1 year ago

Keeping the old structure isn't really possible/desired with the new meson build system, their meson build makes a lot of assumptions and introduces new conventions on the organisation in the file system, which includes the new blocklib/digitizers/... structure. Besides that, the class structure and hierarchy changed also (with the interface header generated by meson/GR from the .yml files, and our code being in the new foo_cpu.h/cc files), so even if the rough structure could be maintained, the files would still be renamed and change a lot.

I see how that makes code review tricky, but I don't know how to fix that...

Ok, helped myself with cloning the project twice and comparing files in main against its successors in the gr40-port branch. Yea, most structural things have changed, though at least for the logic parts in the *.cc files it helps a bit :slightly_smiling_face: .

include <gnuradio/digitizers/median_and_average.h>

I suppose the headers are now generated from the *.yml files ?

Currently, I have some missing build dependency during meson build, which called pmtf. I suppose I need to build & install the dev-4.0 branch of gnuradio to build this gr40-port branch ? In general, it would be nice to have meson build instructions inside the README.md.

frankosterfeld commented 1 year ago

Currently, I have some missing build dependency during meson build, which called pmtf. I suppose I need to build & install the dev-4.0 branch of gnuradio to build this gr40-port branch ?

You could add a fairly simple wrap for this:

# subprojects/pmtf.wrap

[wrap-git]
url = https://github.com/gnuradio/pmt
revision = HEAD

[provide]
pmtf = pmtf_dep

This alone won't help, the code simply requires Gnu RADIO dev-4.0 branch. I don't think we want that as a subproject but leave it to the user to install separately.