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

Rx decoders #1153

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Rx decoders #1153

wants to merge 10 commits into from

Conversation

mcapdeville
Copy link

Hi,
This patch serie add generic decoders interface to receiver class.

To be able to have more than one digital decoder in rx chain and not to
add a per decoder interface, this patch make a generic interface to
decoders implemented in rx chain.
Decoders are assign an identifier : enum receiver_base_cf::rx_decoder

wfmrx demodulator is tweaked so the rds decoder use this interface.

Then a radiofax and a rtty decoder are added to nbrx demodulator.


Marc F4JMZ

@vladisslav2011
Copy link
Contributor

vladisslav2011 commented Oct 15, 2022

WOW!

I'll try to rebase this on top of my dev branch and test.
It does not build...

In file included from /home/vlad/warez/gqrx-my/src/dsp/rtty/rtty_demod.cpp:24:0:
/home/vlad/warez/gqrx-my/src/dsp/rtty/rtty_demod.h:27:10: fatal error: gnuradio/blocks/sub.h: No such file or directory
 #include <gnuradio/blocks/sub.h>
          ^~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
src/CMakeFiles/gqrx.dir/build.make:433: recipe for target 'src/CMakeFiles/gqrx.dir/dsp/rtty/rtty_demod.cpp.o' failed
make[2]: *** [src/CMakeFiles/gqrx.dir/dsp/rtty/rtty_demod.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
In file included from /home/vlad/warez/gqrx-my/src/receivers/nbrx.h:39:0,
                 from /home/vlad/warez/gqrx-my/src/applications/gqrx/receiver.cpp:39:
/home/vlad/warez/gqrx-my/src/dsp/rtty/rtty_demod.h:27:10: fatal error: gnuradio/blocks/sub.h: No such file or directory
 #include <gnuradio/blocks/sub.h>
          ^~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
src/CMakeFiles/gqrx.dir/build.make:225: recipe for target 'src/CMakeFiles/gqrx.dir/applications/gqrx/receiver.cpp.o' failed
make[2]: *** [src/CMakeFiles/gqrx.dir/applications/gqrx/receiver.cpp.o] Error 1
In file included from /home/vlad/warez/gqrx-my/src/dsp/fax/fax_demod.cpp:24:0:
/home/vlad/warez/gqrx-my/src/dsp/fax/fax_demod.h:27:10: fatal error: gnuradio/filter/freq_xlating_fir_filter.h: No such file or directory
 #include <gnuradio/filter/freq_xlating_fir_filter.h>
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
src/CMakeFiles/gqrx.dir/build.make:628: recipe for target 'src/CMakeFiles/gqrx.dir/dsp/fax/fax_demod.cpp.o' failed
make[2]: *** [src/CMakeFiles/gqrx.dir/dsp/fax/fax_demod.cpp.o] Error 1
In file included from /home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:27:0:
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.h:61:25: error: 'fir_filter' in namespace 'gr::filter::kernel' does not name a template type
     gr::filter::kernel::fir_filter<gr_complex,gr_complex,gr_complex> d_mark_fir;
                         ^~~~~~~~~~
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.h:62:25: error: 'fir_filter' in namespace 'gr::filter::kernel' does not name a template type
     gr::filter::kernel::fir_filter<gr_complex,gr_complex,gr_complex> d_space_fir;
                         ^~~~~~~~~~
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp: In constructor 'gr::rtty::fsk_demod_impl::fsk_demod_impl(float, unsigned int, float, float, float)':
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:40:4: error: class 'gr::rtty::fsk_demod_impl' does not have any field named 'd_mark_fir'
    d_mark_fir(decimation,{}),
    ^~~~~~~~~~
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:41:4: error: class 'gr::rtty::fsk_demod_impl' does not have any field named 'd_space_fir'
    d_space_fir(decimation,{}),
    ^~~~~~~~~~~
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp: In member function 'virtual void gr::rtty::fsk_demod_impl::set_mark_freq(float)':
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:116:20: error: 'GR_M_PI' was not declared in this scope
    float fwT0 = 2* GR_M_PI * (mark_freq/d_sample_rate);
                    ^~~~~~~
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:116:20: note: suggested alternative: 'M_2_PI'
    float fwT0 = 2* GR_M_PI * (mark_freq/d_sample_rate);
                    ^~~~~~~
                    M_2_PI
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:123:4: error: 'd_mark_fir' was not declared in this scope
    d_mark_fir.set_taps(ctaps);
    ^~~~~~~~~~
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:123:4: note: suggested alternative: 'd_mark_freq'
    d_mark_fir.set_taps(ctaps);
    ^~~~~~~~~~
    d_mark_freq
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp: In member function 'virtual void gr::rtty::fsk_demod_impl::set_space_freq(float)':
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:136:20: error: 'GR_M_PI' was not declared in this scope
    float fwT0 = 2* GR_M_PI * (space_freq/d_sample_rate);
                    ^~~~~~~
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:136:20: note: suggested alternative: 'M_2_PI'
    float fwT0 = 2* GR_M_PI * (space_freq/d_sample_rate);
                    ^~~~~~~
                    M_2_PI
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:143:4: error: 'd_space_fir' was not declared in this scope
    d_space_fir.set_taps(ctaps);
    ^~~~~~~~~~~
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:143:4: note: suggested alternative: 'd_space_freq'
    d_space_fir.set_taps(ctaps);
    ^~~~~~~~~~~
    d_space_freq
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp: In member function 'virtual int gr::rtty::fsk_demod_impl::work(int, gr_vector_const_void_star&, gr_vector_void_star&)':
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:177:15: error: 'd_mark_fir' was not declared in this scope
     mark[i] = d_mark_fir.filter(&in[j]);
               ^~~~~~~~~~
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:177:15: note: suggested alternative: 'd_mark_freq'
     mark[i] = d_mark_fir.filter(&in[j]);
               ^~~~~~~~~~
               d_mark_freq
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:178:16: error: 'd_space_fir' was not declared in this scope
     space[i] = d_space_fir.filter(&in[j]);
                ^~~~~~~~~~~
/home/vlad/warez/gqrx-my/src/dsp/rtty/fsk_demod_impl.cpp:178:16: note: suggested alternative: 'd_space_freq'
     space[i] = d_space_fir.filter(&in[j]);
                ^~~~~~~~~~~
                d_space_freq
src/CMakeFiles/gqrx.dir/build.make:446: recipe for target 'src/CMakeFiles/gqrx.dir/dsp/rtty/fsk_demod_impl.cpp.o' failed

@vladisslav2011
Copy link
Contributor

vladisslav2011 commented Oct 15, 2022

I think, it may be better to fix individual commits (tabs to spaces, minimize boost usage, fix compatibility with newer and older GNU Radio versions)
And I think, it may be better to reuse AFSK1200 decoder window instead of adding more docks. I don't think, that AFSK1200, WEFAX and RTTY decoders may run at the same time on the same channel...

@mcapdeville mcapdeville force-pushed the rx_decoders branch 3 times, most recently from d1f4ae5 to 07625d8 Compare October 16, 2022 17:54
@vladisslav2011
Copy link
Contributor

The CI still fails.
gr::filter::kernel::fir_filter does not support setting decimation (for obvious reasons).
You are already decimating the signal here https://github.com/mcapdeville/gqrx/blob/rx_decoders/src/dsp/rtty/fsk_demod_impl.cpp#L185

I've tested the RTTY decoder and it sort-of works on strong signals. It produces many garbage symbols from noise when there is no signal and does not decode weak signals well. Carrier detector missing?
I think, it may be better to use existing NFM demodulator output instead of reimplementing FSK/NFM demodulators inside of decoders.

@vladisslav2011
Copy link
Contributor

The CI is passing. You may ignore MacOS build if you do not have a developer certificate.

@vladisslav2011
Copy link
Contributor

vladisslav2011 commented Oct 16, 2022

Testing (2/10): https://github.com/vladisslav2011/gqrx/tree/rx_decoders_test
The RDS decoder works.
Testing (6/10)
The FAX decoder works:
fax_decoder

@vladisslav2011
Copy link
Contributor

Testing (10/10).
WEFAX decoder works perfectly.
https://www.youtube.com/watch?v=hNO8uMBG8m0
RTTY decoder produces a lot of garbage and fails to pick weak transmissions.
https://www.youtube.com/watch?v=qPszWRXI9NU

@willcode
Copy link
Contributor

Wow, quite the patch! I wonder if we should have the GR blocks as a separate package somehow. They seem like they would be useful to other projects. No specific ideas on how to do this.

@mcapdeville mcapdeville force-pushed the rx_decoders branch 2 times, most recently from 208e43a to 104f934 Compare October 18, 2022 07:03
@mcapdeville
Copy link
Author

Hi, thanks for your comments.
I have updated RTTY decoder to improve sensitivity by normalizing power. the asynchronous decoder now have moving threshold helping getting bits when selectives fadings are present.
RTTY garbage on no or weak signal can be silent using the squelch.
dsp/rtty and dsp/fax blocks does not have dependencies on gqrx, so they can be used as-is in other gnuradio project (c++).


Marc F4JMZ

@vladisslav2011
Copy link
Contributor

vladisslav2011 commented Oct 18, 2022

Hello.
Thanks. I'll pull your latest changes and test.

The main problem is not the sensitivity. The main problem is incorrect filtering... The RRC filter does not fit well here.
Just replacing the RRC with a complex bandpass makes FSK demodulator work much, much better. Most moderate-powered signals can be decoded and some weak signals can be decoded too (with a lot of garbage).
Common Gqrx simple_squelch/pwr_squelch does not work well on FSK signals. It either opens too often (on pulsed/wideband interference), producing garbage or does not open on a weak signal. I think, the squelch should operate on space-mark difference instead of absolute level.
I think, if a signal can be "decoded visually" from the waterfall (see picture), it should be decoded perfectly by the decoder.
bandpass

More possible RTTY decoder improvements:

Better symbol synchronization: delay decoding by 10 symbols; check, that there are 10 valid, almost noise-free symbols in a buffer before switching to a CD state; check that next 10 symbols does not look valid and exit CD state.

Save settings to a config file and restore settings at startup.

Carrier detection and logging timestamps and signal power ("\n2022-10-18T00:00:00Z [30db] " style prefix on CD).

An option to enable unattended logging to a text file on GNU Radio side (to make it more compatible with multi VFO implementations). Suggested file name template: "gqrx_[date]_[freq]_rtty.txt". An option to select a directory to write RTTY log files.

@rag2
Copy link

rag2 commented Oct 22, 2022 via email

@mcapdeville
Copy link
Author

Hi, what's news :

  • Fax and rtty decoders now save their settings to default configuration file.
  • In rtty decoder, fsk demodulator is now using two simple BPF made from a translated float taps LPF. Band width, transition width and number of taps (filter len) are controllable from the gui. the filter length is given in number of symbol (1.0 by default). A threshold is apply to power ratio of the two tones filters output to distinguish tones from noise. A carrier detect is implemented and output to the async decoder. The carrier detect is evaluated on word len symbols minus the stop bit, so we dont miss the first char. Async decoder output escaped ASCII STX and ETX char to the char_store block to indicate carrier detect state change.
  • rtty decoder have an auto-save option, but use with care, as it can produce a lot of small files on weird reception.
  • rtty gui have an increased number of parameters, so I've implemented a tab widget and a profile management.

Marc F4JMZ

@luzpaz
Copy link
Contributor

luzpaz commented Oct 23, 2023

Is there an ETA on this ?

@argilo
Copy link
Member

argilo commented Oct 23, 2023

No, there is not. I'll review this when I get a chance, but I don't know when that will be.

std::mutex d_mutex;

int d_len;
boost::circular_buffer<std::vector<unsigned char>> d_data;
Copy link
Member

@argilo argilo Nov 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gqrx does not use boost (except boost::shared_ptr, which is required to support GNU Radio 3.8, and will be removed as soon as Gqrx stops supporting 3.8). I don't want to add back boost as a dependency, so this will need to be replaced with either something in the standard library, or else a GNU Radio circular buffer.


void store(std::string data);
std::mutex d_mutex;
boost::circular_buffer<std::string> d_data;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, this should be replaced with something else.

int cd_count;


boost::circular_buffer<char> last_samples;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, this should be replaced with something else.

#include "fax_demod.h"
#include "dsp/fax/fax_decoder.h"
#include "dsp/fax/fax_store.h"
#include "vector"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include "vector"
#include <vector>

@argilo
Copy link
Member

argilo commented Nov 4, 2023

Some errors are printed during startup:

qt.core.qmetaobject.connectslotsbyname: QMetaObject::connectSlotsByName: No matching signal for on_bandwidth_valueChanged(int)
qt.core.qmetaobject.connectslotsbyname: QMetaObject::connectSlotsByName: No matching signal for on_transwidth_valueChanged(int)
qt.core.qmetaobject.connectslotsbyname: QMetaObject::connectSlotsByName: No matching signal for on_filterlen_valueChanged(int)

@argilo
Copy link
Member

argilo commented Nov 4, 2023

Gqrx now warns about implicit promotions from float to double (-Wdouble-promotion), because this has proven to be a useful way of identifying DSP performance problems. This pull request introduces many new warnings. If it's too tedious to clean them up, I can take care of it myself before merging this.

@argilo
Copy link
Member

argilo commented Nov 4, 2023

Valgrind reports various errors. They appear to be due to various variables (e.g. last_bit) being read before they have been initialized:

==246831== Conditional jump or move depends on uninitialised value(s)
==246831==    at 0x214B00: gr::rtty::fsk_demod_impl::work(int, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) (fsk_demod_impl.cpp:376)
==246831==    by 0x66F8D8A: gr::sync_decimator::general_work(int, std::vector<int, std::allocator<int> >&, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) (sync_decimator.cc:50)
==246831==    by 0x66A239A: gr::block_executor::run_one_iteration() (block_executor.cc:623)
==246831==    by 0x670F57A: gr::tpb_thread_body::tpb_thread_body(std::shared_ptr<gr::block>, std::shared_ptr<boost::barrier>, int) (tpb_thread_body.cc:94)
==246831==    by 0x66F7359: operator() (scheduler_tpb.cc:38)
==246831==    by 0x66F7359: operator() (thread_body_wrapper.h:49)
==246831==    by 0x66F7359: __invoke_impl<void, gr::thread::thread_body_wrapper<gr::tpb_container>&> (invoke.h:61)
==246831==    by 0x66F7359: __invoke_r<void, gr::thread::thread_body_wrapper<gr::tpb_container>&> (invoke.h:111)
==246831==    by 0x66F7359: std::_Function_handler<void (), gr::thread::thread_body_wrapper<gr::tpb_container> >::_M_invoke(std::_Any_data const&) (std_function.h:290)
==246831==    by 0x84D00CA: ??? (in /usr/lib/x86_64-linux-gnu/libboost_thread.so.1.74.0)
==246831==    by 0x6E93AC2: start_thread (pthread_create.c:442)
==246831==    by 0x6F24BF3: clone (clone.S:100)
==246831== 
==246831== Conditional jump or move depends on uninitialised value(s)
==246831==    at 0x214718: gr::rtty::fsk_demod_impl::work(int, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) (fsk_demod_impl.cpp:400)
==246831==    by 0x66F8D8A: gr::sync_decimator::general_work(int, std::vector<int, std::allocator<int> >&, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) (sync_decimator.cc:50)
==246831==    by 0x66A239A: gr::block_executor::run_one_iteration() (block_executor.cc:623)
==246831==    by 0x670F57A: gr::tpb_thread_body::tpb_thread_body(std::shared_ptr<gr::block>, std::shared_ptr<boost::barrier>, int) (tpb_thread_body.cc:94)
==246831==    by 0x66F7359: operator() (scheduler_tpb.cc:38)
==246831==    by 0x66F7359: operator() (thread_body_wrapper.h:49)
==246831==    by 0x66F7359: __invoke_impl<void, gr::thread::thread_body_wrapper<gr::tpb_container>&> (invoke.h:61)
==246831==    by 0x66F7359: __invoke_r<void, gr::thread::thread_body_wrapper<gr::tpb_container>&> (invoke.h:111)
==246831==    by 0x66F7359: std::_Function_handler<void (), gr::thread::thread_body_wrapper<gr::tpb_container> >::_M_invoke(std::_Any_data const&) (std_function.h:290)
==246831==    by 0x84D00CA: ??? (in /usr/lib/x86_64-linux-gnu/libboost_thread.so.1.74.0)
==246831==    by 0x6E93AC2: start_thread (pthread_create.c:442)
==246831==    by 0x6F24BF3: clone (clone.S:100)
==246831== 
==246831== Conditional jump or move depends on uninitialised value(s)
==246831==    at 0x2145C7: gr::rtty::fsk_demod_impl::work(int, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) (fsk_demod_impl.cpp:405)
==246831==    by 0x66F8D8A: gr::sync_decimator::general_work(int, std::vector<int, std::allocator<int> >&, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) (sync_decimator.cc:50)
==246831==    by 0x66A239A: gr::block_executor::run_one_iteration() (block_executor.cc:623)
==246831==    by 0x670F57A: gr::tpb_thread_body::tpb_thread_body(std::shared_ptr<gr::block>, std::shared_ptr<boost::barrier>, int) (tpb_thread_body.cc:94)
==246831==    by 0x66F7359: operator() (scheduler_tpb.cc:38)
==246831==    by 0x66F7359: operator() (thread_body_wrapper.h:49)
==246831==    by 0x66F7359: __invoke_impl<void, gr::thread::thread_body_wrapper<gr::tpb_container>&> (invoke.h:61)
==246831==    by 0x66F7359: __invoke_r<void, gr::thread::thread_body_wrapper<gr::tpb_container>&> (invoke.h:111)
==246831==    by 0x66F7359: std::_Function_handler<void (), gr::thread::thread_body_wrapper<gr::tpb_container> >::_M_invoke(std::_Any_data const&) (std_function.h:290)
==246831==    by 0x84D00CA: ??? (in /usr/lib/x86_64-linux-gnu/libboost_thread.so.1.74.0)
==246831==    by 0x6E93AC2: start_thread (pthread_create.c:442)
==246831==    by 0x6F24BF3: clone (clone.S:100)
==246831== 
==246831== Conditional jump or move depends on uninitialised value(s)
==246831==    at 0x217227: gr::rtty::async_rx_impl::general_work(int, std::vector<int, std::allocator<int> >&, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) (async_rx_impl.cpp:126)
==246831==    by 0x66A239A: gr::block_executor::run_one_iteration() (block_executor.cc:623)
==246831==    by 0x670F57A: gr::tpb_thread_body::tpb_thread_body(std::shared_ptr<gr::block>, std::shared_ptr<boost::barrier>, int) (tpb_thread_body.cc:94)
==246831==    by 0x66F7359: operator() (scheduler_tpb.cc:38)
==246831==    by 0x66F7359: operator() (thread_body_wrapper.h:49)
==246831==    by 0x66F7359: __invoke_impl<void, gr::thread::thread_body_wrapper<gr::tpb_container>&> (invoke.h:61)
==246831==    by 0x66F7359: __invoke_r<void, gr::thread::thread_body_wrapper<gr::tpb_container>&> (invoke.h:111)
==246831==    by 0x66F7359: std::_Function_handler<void (), gr::thread::thread_body_wrapper<gr::tpb_container> >::_M_invoke(std::_Any_data const&) (std_function.h:290)
==246831==    by 0x84D00CA: ??? (in /usr/lib/x86_64-linux-gnu/libboost_thread.so.1.74.0)
==246831==    by 0x6E93AC2: start_thread (pthread_create.c:442)
==246831==    by 0x6F24BF3: clone (clone.S:100)
==246831== 
==246831== Conditional jump or move depends on uninitialised value(s)
==246831==    at 0x217231: gr::rtty::async_rx_impl::general_work(int, std::vector<int, std::allocator<int> >&, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) (async_rx_impl.cpp:132)
==246831==    by 0x66A239A: gr::block_executor::run_one_iteration() (block_executor.cc:623)
==246831==    by 0x670F57A: gr::tpb_thread_body::tpb_thread_body(std::shared_ptr<gr::block>, std::shared_ptr<boost::barrier>, int) (tpb_thread_body.cc:94)
==246831==    by 0x66F7359: operator() (scheduler_tpb.cc:38)
==246831==    by 0x66F7359: operator() (thread_body_wrapper.h:49)
==246831==    by 0x66F7359: __invoke_impl<void, gr::thread::thread_body_wrapper<gr::tpb_container>&> (invoke.h:61)
==246831==    by 0x66F7359: __invoke_r<void, gr::thread::thread_body_wrapper<gr::tpb_container>&> (invoke.h:111)
==246831==    by 0x66F7359: std::_Function_handler<void (), gr::thread::thread_body_wrapper<gr::tpb_container> >::_M_invoke(std::_Any_data const&) (std_function.h:290)
==246831==    by 0x84D00CA: ??? (in /usr/lib/x86_64-linux-gnu/libboost_thread.so.1.74.0)
==246831==    by 0x6E93AC2: start_thread (pthread_create.c:442)
==246831==    by 0x6F24BF3: clone (clone.S:100)
==246831== 
==246831== Conditional jump or move depends on uninitialised value(s)
==246831==    at 0x21723B: gr::rtty::async_rx_impl::general_work(int, std::vector<int, std::allocator<int> >&, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) (async_rx_impl.cpp:138)
==246831==    by 0x66A239A: gr::block_executor::run_one_iteration() (block_executor.cc:623)
==246831==    by 0x670F57A: gr::tpb_thread_body::tpb_thread_body(std::shared_ptr<gr::block>, std::shared_ptr<boost::barrier>, int) (tpb_thread_body.cc:94)
==246831==    by 0x66F7359: operator() (scheduler_tpb.cc:38)
==246831==    by 0x66F7359: operator() (thread_body_wrapper.h:49)
==246831==    by 0x66F7359: __invoke_impl<void, gr::thread::thread_body_wrapper<gr::tpb_container>&> (invoke.h:61)
==246831==    by 0x66F7359: __invoke_r<void, gr::thread::thread_body_wrapper<gr::tpb_container>&> (invoke.h:111)
==246831==    by 0x66F7359: std::_Function_handler<void (), gr::thread::thread_body_wrapper<gr::tpb_container> >::_M_invoke(std::_Any_data const&) (std_function.h:290)
==246831==    by 0x84D00CA: ??? (in /usr/lib/x86_64-linux-gnu/libboost_thread.so.1.74.0)
==246831==    by 0x6E93AC2: start_thread (pthread_create.c:442)
==246831==    by 0x6F24BF3: clone (clone.S:100)
==246831== 

@mcapdeville
Copy link
Author

Hi,
I have follow your request, so there is no more boost::circular_buffer dependencies. I've also corrected the float to double promotion warning and the uninitialized variable in fsk_demod_impl.cpp and async_rx_impl.cpp. Somme slot prototype in dock_rtty have been corrected to avoid the unmatched signal message at startup.

thanks for your revue.


Marc F4JMZ

@argilo
Copy link
Member

argilo commented Nov 4, 2023

Thanks for making those improvements.

Unfortunately the updated code does not compile due to the following error: error: use of undeclared identifier 'M_PIf'.

@mcapdeville
Copy link
Author

Sorry, It seems that M_PIf is not define on all platform. It is on debian 12 with libc6-dev:amd64 version 2.36-9+deb12u2.
I add a conditional define then i re-push.

To be able to have more than one digital decoder in rx chain and not to
add a per decoder interface, this patch make a generic interface to
decoders implemented in rx chain.
Decoders are asign an identifier : enum receiver_base_cf::rx_decoder

Function added :
start_decoder(enum rx_decoder decoder_type)
stop_decoder(enum rx_decoder decoder_type)
is_decoder_active(enum rx_decoder decoder_type)
reset_decoder(enum rx_decoder decoder_type)
get_decoder_data(enum rx_decoder decoder_type,void* , int&)
set_decoder_param(enum rx_decoder decoder_type, std::string param, std::string val);
get_decoder_param(enum rx_decoder decoder_type, std::string param, std::string& val);

in receiver_base_cf class :
decoder_type is one of enum receiver_base_cf::rx_decoder :
	RX_DECODER_ALL, RX_DECODER_NONE, RX_DECODER_ANY,
follows by any added decoder type, ending with RX_DECODER_MAX.

multiplexing between receiver::rx_decoder and rx_chain dependent decoder
type is done in applications/gqrx/receiver.cpp

RX_DECODER_ALL and RX_DECODER_ANY are for convenience in statement
   if (decoder_is_active(RX_DECODER_ANY) reset_decoder(RX_DECODER_ALL
)
decoder type for rds decoder is RX_DECODER_RDS

Function renamed :
get_rds_data(std::string, int&) -> get_decoder_data(RX_DECODER_RDS,void* , int&)
start_rds_decoder() -> start_decoder(RX_DECODER_RDS)
stop_rds_decoder() -> stop_decoder(RX_DECODER_RDS)
reset_rds_parser() -> reset_decoder(RX_DECODER_RDS)
is_rds_decoder_active() -> is_decoder_active(RX_DECODER_RDS)
@mcapdeville
Copy link
Author

There was other float to double implicit promotion that i've corrected in this last push.

@argilo
Copy link
Member

argilo commented Nov 5, 2023

I'm not sure yet exactly how I triggered this, but I got a heap buffer overflow:

=================================================================
==395771==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61e0000233d0 at pc 0x5573024ba33c bp 0x7fbc54e2cb10 sp 0x7fbc54e2cb00
READ of size 8 at 0x61e0000233d0 thread T131 (fax_store35)
    #0 0x5573024ba33b in std::vector<unsigned char, std::allocator<unsigned char> >::capacity() const /usr/include/c++/11/bits/stl_vector.h:999
    #1 0x5573024ba33b in std::vector<unsigned char, std::allocator<unsigned char> >::operator=(std::vector<unsigned char, std::allocator<unsigned char> > const&) /usr/include/c++/11/bits/vector.tcc:224
    #2 0x5573024ba33b in gr::fax::fax_store::d_data_push(std::vector<unsigned char, std::allocator<unsigned char> > const&) /home/argilo/prefix_310/src/gqrx/src/dsp/fax/fax_store.cpp:154
    #3 0x5573024bf77d in gr::fax::fax_store::work(int, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) /home/argilo/prefix_310/src/gqrx/src/dsp/fax/fax_store.cpp:73
    #4 0x7fbc9f3429aa in gr::sync_block::general_work(int, std::vector<int, std::allocator<int> >&, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) /home/argilo/prefix_310/src/gnuradio/gnuradio-runtime/lib/sync_block.cc:49
    #5 0x7fbc9f2ec39a in gr::block_executor::run_one_iteration() /home/argilo/prefix_310/src/gnuradio/gnuradio-runtime/lib/block_executor.cc:623
    #6 0x7fbc9f35957a in gr::tpb_thread_body::tpb_thread_body(std::shared_ptr<gr::block>, std::shared_ptr<boost::barrier>, int) /home/argilo/prefix_310/src/gnuradio/gnuradio-runtime/lib/tpb_thread_body.cc:94
    #7 0x7fbc9f341359 in gr::tpb_container::operator()() /home/argilo/prefix_310/src/gnuradio/gnuradio-runtime/lib/scheduler_tpb.cc:38
    #8 0x7fbc9f341359 in gr::thread::thread_body_wrapper<gr::tpb_container>::operator()() /home/argilo/prefix_310/src/gnuradio/gnuradio-runtime/lib/../include/gnuradio/thread/thread_body_wrapper.h:49
    #9 0x7fbc9f341359 in void std::__invoke_impl<void, gr::thread::thread_body_wrapper<gr::tpb_container>&>(std::__invoke_other, gr::thread::thread_body_wrapper<gr::tpb_container>&) /usr/include/c++/11/bits/invoke.h:61
    #10 0x7fbc9f341359 in std::enable_if<is_invocable_r_v<void, gr::thread::thread_body_wrapper<gr::tpb_container>&>, void>::type std::__invoke_r<void, gr::thread::thread_body_wrapper<gr::tpb_container>&>(gr::thread::thread_body_wrapper<gr::tpb_container>&) /usr/include/c++/11/bits/invoke.h:111
    #11 0x7fbc9f341359 in std::_Function_handler<void (), gr::thread::thread_body_wrapper<gr::tpb_container> >::_M_invoke(std::_Any_data const&) /usr/include/c++/11/bits/std_function.h:290
    #12 0x7fbca0d890ca  (/lib/x86_64-linux-gnu/libboost_thread.so.1.74.0+0x150ca)
    #13 0x7fbc9de94ac2 in start_thread nptl/pthread_create.c:442
    #14 0x7fbc9df26a3f  (/lib/x86_64-linux-gnu/libc.so.6+0x126a3f)

0x61e0000233d0 is located 16 bytes to the right of 2880-byte region [0x61e000022880,0x61e0000233c0)
allocated by thread T0 here:
    #0 0x7fbca14b61e7 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x5573024bc019 in __gnu_cxx::new_allocator<std::vector<unsigned char, std::allocator<unsigned char> > >::allocate(unsigned long, void const*) /usr/include/c++/11/ext/new_allocator.h:127
    #2 0x5573024bc019 in std::allocator_traits<std::allocator<std::vector<unsigned char, std::allocator<unsigned char> > > >::allocate(std::allocator<std::vector<unsigned char, std::allocator<unsigned char> > >&, unsigned long) /usr/include/c++/11/bits/alloc_traits.h:464
    #3 0x5573024bc019 in std::_Vector_base<std::vector<unsigned char, std::allocator<unsigned char> >, std::allocator<std::vector<unsigned char, std::allocator<unsigned char> > > >::_M_allocate(unsigned long) /usr/include/c++/11/bits/stl_vector.h:346
    #4 0x5573024bc019 in std::_Vector_base<std::vector<unsigned char, std::allocator<unsigned char> >, std::allocator<std::vector<unsigned char, std::allocator<unsigned char> > > >::_M_allocate(unsigned long) /usr/include/c++/11/bits/stl_vector.h:343
    #5 0x5573024bc019 in std::_Vector_base<std::vector<unsigned char, std::allocator<unsigned char> >, std::allocator<std::vector<unsigned char, std::allocator<unsigned char> > > >::_M_create_storage(unsigned long) /usr/include/c++/11/bits/stl_vector.h:361
    #6 0x5573024bc019 in std::_Vector_base<std::vector<unsigned char, std::allocator<unsigned char> >, std::allocator<std::vector<unsigned char, std::allocator<unsigned char> > > >::_Vector_base(unsigned long, std::allocator<std::vector<unsigned char, std::allocator<unsigned char> > > const&) /usr/include/c++/11/bits/stl_vector.h:305
    #7 0x5573024bc019 in std::vector<std::vector<unsigned char, std::allocator<unsigned char> >, std::allocator<std::vector<unsigned char, std::allocator<unsigned char> > > >::vector(unsigned long, std::allocator<std::vector<unsigned char, std::allocator<unsigned char> > > const&) /usr/include/c++/11/bits/stl_vector.h:511
    #8 0x5573024bc019 in gr::fax::fax_store::fax_store(int) /home/argilo/prefix_310/src/gqrx/src/dsp/fax/fax_store.cpp:42

Thread T131 (fax_store35) created by T0 here:
    #0 0x7fbca1458685 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:216
    #1 0x7fbca0d851f3 in boost::thread::start_thread_noexcept() (/lib/x86_64-linux-gnu/libboost_thread.so.1.74.0+0x111f3)

SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/include/c++/11/bits/stl_vector.h:999 in std::vector<unsigned char, std::allocator<unsigned char> >::capacity() const
Shadow bytes around the buggy address:
  0x0c3c7fffc620: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3c7fffc630: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3c7fffc640: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3c7fffc650: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3c7fffc660: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c3c7fffc670: 00 00 00 00 00 00 00 00 fa fa[fa]fa fa fa fa fa
  0x0c3c7fffc680: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3c7fffc690: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3c7fffc6a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3c7fffc6b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c3c7fffc6c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==395771==ABORTING

@argilo
Copy link
Member

argilo commented Nov 5, 2023

It looks like you implemented your own queue from scratch to replace boost::circular_buffer. Would std::queue do what you want?

@argilo
Copy link
Member

argilo commented Nov 5, 2023

An error from UBSAN:

/home/argilo/prefix_310/src/gqrx/src/dsp/fax/fax_store.cpp:106:12: runtime error: null pointer passed as argument 2, which is declared to never be null

@argilo
Copy link
Member

argilo commented Nov 5, 2023

ASAN reports a memory leak:

==407951==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 2265 byte(s) in 4 object(s) allocated from:
    #0 0x7fee4dab61e7 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x55969fddd5aa in __gnu_cxx::new_allocator<unsigned char>::allocate(unsigned long, void const*) /usr/include/c++/11/ext/new_allocator.h:127
    #2 0x55969fddd5aa in std::allocator_traits<std::allocator<unsigned char> >::allocate(std::allocator<unsigned char>&, unsigned long) /usr/include/c++/11/bits/alloc_traits.h:464
    #3 0x55969fddd5aa in std::_Vector_base<unsigned char, std::allocator<unsigned char> >::_M_allocate(unsigned long) /usr/include/c++/11/bits/stl_vector.h:346
    #4 0x55969fddd5aa in unsigned char* std::vector<unsigned char, std::allocator<unsigned char> >::_M_allocate_and_copy<__gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char> > > >(unsigned long, __gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char> > >, __gnu_cxx::__normal_iterator<unsigned char const*, std::vector<unsigned char, std::allocator<unsigned char> > >) /usr/include/c++/11/bits/stl_vector.h:1511
    #5 0x55969fddd5aa in std::vector<unsigned char, std::allocator<unsigned char> >::operator=(std::vector<unsigned char, std::allocator<unsigned char> > const&) /usr/include/c++/11/bits/vector.tcc:226
    #6 0x55969fddd5aa in gr::fax::fax_store::d_data_push(std::vector<unsigned char, std::allocator<unsigned char> > const&) /home/argilo/prefix_310/src/gqrx/src/dsp/fax/fax_store.cpp:154

@argilo
Copy link
Member

argilo commented Nov 5, 2023

By the way, if you want to test your code with ASAN and UBSAN, you can add the following to src/CMakeLists.txt:

set(CMAKE_C_FLAGS "-fsanitize=address -fsanitize=undefined ${CMAKE_C_FLAGS}")
set(CMAKE_CXX_FLAGS "-fsanitize=address -fsanitize=undefined ${CMAKE_CXX_FLAGS}")
set(CMAKE_EXE_LINKER_FLAGS "-fsanitize=address -fsanitize=undefined ${CMAKE_EXE_LINKER_FLAGS}")

@mcapdeville
Copy link
Author

It seems that all these error are due to my implementation of circular buffer. I replace it by std::queue in this rewrite.

Decoder_type for fax decoder is RX_DECODER_FAX
    Dockable window to control fax decoder and display received image.
This is the rtty decoder dsp block for gqrx

This block is to be plug in nbrx rx_chain.
Windows to control rtty decoder and display decoded text.
@mcapdeville
Copy link
Author

And more float to double conversion that trigger warning on MacOs CI only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants