f4exb / sdrangel

SDR Rx/Tx software for Airspy, Airspy HF+, BladeRF, HackRF, LimeSDR, PlutoSDR, RTL-SDR, SDRplay and FunCube
GNU General Public License v3.0
2.92k stars 441 forks source link

M17 Plug-in not showing in source built SDRAngel #1364

Closed alphafox02 closed 2 years ago

alphafox02 commented 2 years ago

Setup: Built according to Linux compile guide on 20.04

Expectation: M17 demod available under channels

Result: Freedv demod available, M17 plug-in no where to be found.

Checking terminal reveals this.

2022-07-18 15:24:46.933 (W) PluginManager::loadPluginsDir: Cannot load library /opt/install/sdrangel/lib/sdrangel/plugins/libmodm17.so: (/opt/install/sdrangel/lib/sdrangel/libmodems.so: undefined symbol: _ZN8modemm1713ClockRecoveryILm48000ELm4800EE9MIN_CLOCKE)

I figured maybe a new ticket will clarify my findings. I’m not using the DEB package which I understand does not have the M17 or FreeDV plug-ins working.

srcejon commented 2 years ago

I see similar problem on 20.04 and 21.10, but 22.04 is OK.

f4exb commented 2 years ago

M17 code will be completely removed in the next release. I am really fed up with this M17 story!

alphafox02 commented 2 years ago

Didn’t mean to add to the story, just trying to clarify if I was doing something wrong on my end. Good to know it works in 22.04, I’ll look at that and see what would keep it working in previous versions. I’ll go ahead and close this I think since now I understand?

alphafox02 commented 2 years ago

I’ll try to find a solution and wait for possibly future changes to SDRAngel. Thank you.

alphafox02 commented 2 years ago

@srcejon

Not to dwell on this, but one other question since you mentioned 22.04 is okay.. is there a particular library that this gets linked to in 22.04 that perhaps I could compile additionally in /opt and link SDRAngel together with in 20.04? I realize this may not be standard, but figured it’s worth a try, maybe? If not, I’ll just drop the effort and roll with what’s available which is working great.

srcejon commented 2 years ago

The problem isn't with a missing library, as far as I can see, it's something to do with the way the libmodems.so (that is part of SDRangel) is being compiled.

E.g. the error error above for 21.10 is:

/opt/install/sdrangel/lib/sdrangel/plugins/libmodm17.so: (/opt/install/sdrangel/lib/sdrangel/libmodems.so: undefined symbol: _ZN8modemm1713ClockRecoveryILm48000ELm4800EE9MIN_CLOCKE)

If we look in the library for the undefined symbol with nm, it is there, but marked as undefined (U):

nm /opt/install/sdrangel/lib/sdrangel/libmodemm17.so | grep _ZN8modemm1713ClockRecoveryILm48000ELm4800EE9MIN_CLOCKE U _ZN8modemm1713ClockRecoveryILm48000ELm4800EE9MIN_CLOCKE

Running with the demangle option, gives the actual variable name:

nm -C /opt/install/sdrangel/lib/sdrangel/libmodemm17.so | grep MIN_CLOCK U modemm17::ClockRecovery<48000ul, 4800ul>::MIN_CLOCK

I can't see why this would be in there as undefined. Other similar variables in that same function don't have symbols (E.g. SAMPLES_PER_SYMBOL or MAX_OFFSET). Looking in libmodems.so compiled under 22.04, the symbol doesn't exist. Seems like a compiler / optimisation problem to me.

alphafox02 commented 2 years ago

Ah I see, and if I compiler on 22.04 the libmodem wouldn’t work on 20.04 cause its linked against all the libraries, which may have different .so names then in 22.04 I suppose among other differences. Darn, that there could be some way to rig something up.

Oh what if I somehow (assuming it’s possible) get the matching version compiler from 22.04 into 20.04 just to build things.

alphafox02 commented 2 years ago

Darn, installed gcc/g++ 11 thinking it’d make a difference but the grep result is the same.

srcejon commented 2 years ago

Actually, the symbols being undefined is probably correct - just in some cases, gcc has optimised them away, so the problem is hidden.

static constexprs need to be defined, but in some cases are just in the .hs.

I see f4exb has fixed this for LinkSetupFrame.h, which was causing a problem on 20.04 (so if you git pull and rebuild, it may work), but looks like there are other cases (that may come and go with different versions of gcc). We'll need a ClockRecovery.cpp being added with static constexpr float MIN_CLOCK to fix the problem above - and quite a lot of the other files by the looks of it.

alphafox02 commented 2 years ago

I only had the clock thing above, but now I’m trying to patiently build gcc12 which I guess is on 22.04, even if it doesn’t help I’m at least learning something new :)

alphafox02 commented 2 years ago

I tried with gcc12 directly, really just rolling the dice cause I'm not sure how to really do this manually.

/opt/build/sdrangel/modemm17$ sudo /usr/local/bin/c++ *.cpp -fPIC -isystem /usr/include/x86_64-linux-gnu/qt5/ -isystem /usr/include/x86_64-linux-gnu/qt5/QtCore -lQt5Core -shared

Googled and made up the compile options till it actually compiled the a.out, renamed it to the .so file and loaded up SDRAngel, still had the same clock unnamed thing. That was fun though... thought I might get lucky.

f4exb commented 2 years ago

We'll need a ClockRecovery.cpp being added with static constexpr float MIN_CLOCK to fix the problem above

ClockRecovery is a class template (see: https://github.com/f4exb/sdrangel/blob/master/modemm17/ClockRecovery.h#L40) so how could you possibly do this? I did not have the same issue with LinkSetupFrame that is not a template.

Note that the issue with LinkSetupFrame was revealed using clang for the compiler. It does not show with gcc.

f4exb commented 2 years ago

I have taken the M17 protocol specific code from https://github.com/mobilinkd/m17-cxx-demod and the general philosophy of this project seems to be "C++ = templates". I already removed the FloatType parameter which removed a few templates. Nobody will ever want to use double precision for this purpose so floating point types can just be float. In the case of ClockRecovery the templates parameters are sample rate and baud rate that essentially are run time parameters thus not static and therefore have nothing to do in a template...

srcejon commented 2 years ago

Note that the issue with LinkSetupFrame was revealed using clang for the compiler. It does not show with gcc.

It was resulting in a similar undefined error to the above for me with gcc on 20.04 - but it's quite dependent on compiler optimisation.

We'll need a ClockRecovery.cpp being added with static constexpr float MIN_CLOCK to fix the problem above

ClockRecovery is a class template (see: https://github.com/f4exb/sdrangel/blob/master/modemm17/ClockRecovery.h#L40) so how could you possibly do this? I did not have the same issue with LinkSetupFrame that is not a template.

Here's an example for templates:

#include <complex>
using namespace std;

template <typename t>
struct test
{
    static constexpr t value = 0.0003;
};

#if DEFINE_VALUE
template <typename t>   
constexpr t test<t>::value;   // Note you don't need to duplicate the value
#endif

int main()
{
    complex<double> x = complex<double>(0,1)*test<double>::value; 
    return 0;
}

If you compile with optimisation:

 g++ test.c -O2

It will compile/link successfully, as the struct is optimised away.

However, compiling without optimisation will give you the error:

  g++ test.c

  undefined reference to `test<double>::value'

As value isn't being optimised away.

To fix, need to make sure value is defined:

g++ test.c -DDEFINE_VALUE

There is a simpler fix for C++17, you can use inline:

template <typename t>
struct test
{
   static inline constexpr t value = 0.0003;
};

And then we can compile without the definition:

g++ test.c -std=c++17

Compiles successfully.

srcejon commented 2 years ago

I already removed the FloatType parameter which removed a few templates. Nobody will ever want to use double precision for this purpose so floating point types can just be float.

I see that the original project does compile with MSVC, even though it has all the templates. I can't see what is going wrong for us, but presumably it can be made to work.

In terms of maintenance (i.e. importing new versions of upstream code), would it be easier to just enable c++20 compilation for just the m17 code, rather than try to remove it?

f4exb commented 2 years ago

I think the c++ level required should be global. Maybe it can be upgraded to c++17 as most compilers are c++17-able these days. Not so sure about c++20.

alphafox02 commented 2 years ago

Is there any variation of the above command line direct compile that I could try on 20.04 with gcc 12? I’m going to try to force it to use c++20 this evening. So far it “compiles” fine just like it’s always done for me, but when I move it over to an SDRAngel install it complains during loading about that clock thing. Happy to try anything if it helps. If I’m headed down the wrong path, please let me know. It’s the first time I compiled gcc from source, so at least I’m learning something!

srcejon commented 2 years ago

Is there any variation of the above command line direct compile that I could try on 20.04 with gcc 12? I’m going to try to force it to use c++20 this evening. So far it “compiles” fine just like it’s always done for me, but when I move it over to an SDRAngel install it complains during loading about that clock thing. Happy to try anything if it helps. If I’m headed down the wrong path, please let me know. It’s the first time I compiled gcc from source, so at least I’m learning something!

I would say just wait a bit until there's an updated version of the source code.

alphafox02 commented 2 years ago

Sounds like a good idea! Thank you.

f4exb commented 2 years ago

Yes I think that M17 code needs a heavy lifting...

alphafox02 commented 2 years ago

Just some positive feedback. I built SDRAngel from source 10 mins ago on 20.04 with stock gcc etc and m17 is now available as a channel within SDRAngel. No issues loading at least, I’ve not tested anything behind that but it’s now there!

alphafox02 commented 2 years ago

I broadcast a M17 signal with gnuradio and demodulated and listened to it with SDRAngel. Worked really well.

f4exb commented 2 years ago

Good news! I did some changes to get rid of some constexpr and simplify the code but I was pretty blind as gcc11 and clang11 have always worked for me.

alphafox02 commented 2 years ago

This looks to now be resolved. 👍