python/init.py

there's this goddamn (what a design fail on the side of gr-newmod) try: / catch: pass thing around the from _swig import * clause. That is cancer and must go; if the C++ components aren't loadable, there should be a clear and loud exception.

user_frontend.ui / user_frontend.py

The python file is generated from the .ui file using pyuic4. Hence, the .py file should not be tracked within git at all. Instead, add_custom_command to generate the file on the fly when building in the python/GUI/CMakeLists.txt; something like

      add_custom_command(OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/user_frontend.py"
                         COMMAND pyuic4 -o "${CMAKE_CURRENT_BINARY_DIR}/user_frontend.py" "${CMAKE_CURRENT_SOURCE_DIR}/user_frontend.ui"
                         MAIN_DEPENDENCY "${CMAKE_CURRENT_SOURCE_DIR}/user_frontend.ui")

and of course, then replacing user_frontend.py in the GR_PYTHON_INSTALL by "${CMAKE_CURRENT_BINARY_DIR}/user_frontend.py"

MLsmd commented 6 years ago

Thanks for the review.! I did the requested changes and some more wrap up, docu, ... I think we are ready for merge in master now.

marcusmueller commented 6 years ago

So, formally OK, now we just have to fix the build failures:

../lib/fib_sink_vb_impl.cc:84:23: error: return type of out-of-line definition of 'gr::dab::fib_sink_vb_impl::process_fig' differs from that in the declaration
    fib_sink_vb_impl::process_fig(uint8_t type, const char *data, uint8_t length) {
                      ^
../lib/fib_sink_vb_impl.h:51:11: note: previous declaration is here
      int process_fig(uint8_t type, const char *data, uint8_t length);
      ~~~ ^

I think you meant to use void here, as the actual implementation never returns a value?

marcusmueller commented 6 years ago

(also, hint, maintainers love code that is going to merge smoothly into master, so a git rebase -s ours might be a good idea)

MLsmd commented 6 years ago

I am not yet finished with reformatting and documenting the whole code. I will give you a hint here when I finished the work. And thanks for the tip with rebase -s ours, I am going to try it out :)

MLsmd commented 6 years ago

I am now finally finished with reformatting and documenting the rest of the code and ready for the merge.

marcusmueller commented 6 years ago

Juche! Will review tomorrow :)

kit-cel / gr-dab

Working branch #34

python/init.py

user_frontend.ui / user_frontend.py