Make it work under linux with gqrx/gr-osmosdr#223
Make it work under linux with gqrx/gr-osmosdr#223vladisslav2011 wants to merge 2 commits intoik1xpv:masterfrom
Conversation
howard0su
left a comment
There was a problem hiding this comment.
Thank you for your change. Overall I like the change. But I would prefer you can split your changes into several parts. For example, firmware change, some change in Core, and Linux USB part.
| if (fc != 0.0f) | ||
| { | ||
| std::unique_lock<std::mutex> lk(fc_mutex); | ||
| shift_limited_unroll_C_sse_inp_c((complexf*)buf, len, stateFineTune); |
There was a problem hiding this comment.
I was unable to make decimator work as expected, so I partially removed it. Offset tuning was removed as well.
There was a problem hiding this comment.
Oscar may help and he is expert on that DSP code.
| delete stateFineTune; | ||
| delete r2iqCntrl; | ||
| delete hardware; | ||
| delete fx3; |
There was a problem hiding this comment.
I don't think this class owes the fx3 instance. it is allocated by someone else. better deallocate by the owner.
There was a problem hiding this comment.
That was freed by the owner while threads were still running, resulting in random crashes/freezes. That's why I did this change. I don't think, that it is better to deallocate by the owner. I think that it is better to use std::shared_ptr. Switching to std::shared_ptr may be the next thing to do. But incorrectly using smart pointers may introduce crazy bugs too...
There was a problem hiding this comment.
it should not happen at least for Windows. Can you point me the crash stacks of two threads? I can help debug.
Core/dsp/ringbuffer.h
Outdated
|
|
||
| T* peekWritePtr(int offset) | ||
| { | ||
| std::unique_lock<std::mutex> lk(mutex); |
There was a problem hiding this comment.
why? atomic read doesn't need mutex lock.
There was a problem hiding this comment.
Thanks. This may be removed. I have added mutex locking to every method trying to make it output continous stream of samples.
There was a problem hiding this comment.
Can you repro the issue via a unit test case?
There was a problem hiding this comment.
I'll recheck the original code as buffer overflow was happening in a different place and was overwriting pointers making it crash here too. I've removed spinlock reimplementation as it already exists inside of std::mutex::lock(), added mutex protection to ringbuffer::setBlockSize and increased the block size to avoid overflow. But then I have fixed invalid calculation of buffer size in different place and removed buffer size increase here (note remaining parenthesis).
There was a problem hiding this comment.
I've retested.
Unfortunately, gqrx crashes after sampling rate change.
Here is the backtrace:
Thread 81 "gqrx" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffa51f3700 (LWP 18108)]
__memcpy_ssse3 () at ../sysdeps/x86_64/multiarch/memcpy-ssse3.S:309
309 ../sysdeps/x86_64/multiarch/memcpy-ssse3.S: Нет такого файла или каталога.
(gdb) bt
#0 0x00007ffff2baa92d in __memcpy_ssse3 () at ../sysdeps/x86_64/multiarch/memcpy-ssse3.S:309
#1 0x00007ffff0363128 in fx3handler::PacketRead(unsigned int, unsigned char*, void*) () at /home/vlad/warez/ExtIO_sddc/build/libsddc/libsddc.so.0
#2 0x00007ffff036347e in streaming_read_async_callback () at /home/vlad/warez/ExtIO_sddc/build/libsddc/libsddc.so.0
#3 0x00007ffff0148738 in () at /lib/x86_64-linux-gnu/libusb-1.0.so.0
#4 0x00007ffff014c89c in () at /lib/x86_64-linux-gnu/libusb-1.0.so.0
#5 0x00007ffff014e2b8 in () at /lib/x86_64-linux-gnu/libusb-1.0.so.0
#6 0x00007ffff014824c in () at /lib/x86_64-linux-gnu/libusb-1.0.so.0
#7 0x00007ffff0149130 in libusb_handle_events_timeout_completed () at /lib/x86_64-linux-gnu/libusb-1.0.so.0
#8 0x00007ffff0364ed0 in usb_device_handle_events () at /home/vlad/warez/ExtIO_sddc/build/libsddc/libsddc.so.0
#9 0x00007ffff0362f19 in std::thread::_State_impl<std::thread::_Invoker<std::tuple<fx3handler::StartStream(ringbuffer<short>&, int)::{lambda()#1}> > >::_M_run() ()
at /home/vlad/warez/ExtIO_sddc/build/libsddc/libsddc.so.0
#10 0x00007ffff36c16df in () at /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#11 0x00007ffff2e366db in start_thread (arg=0x7fffa51f3700) at pthread_create.c:463
#12 0x00007ffff2b5f61f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
Core/fft_mt_r2iq.h
Outdated
| void TurnOff(void); | ||
| bool IsOn(void); | ||
| void TurnOn() override; | ||
| void TurnOff(void) override; |
There was a problem hiding this comment.
please keep consistent. I prefer not put void here.
| uint32_t bw; | ||
| r82xx_init(&tuner); | ||
| r82xx_set_bandwidth(&tuner, 8*1000*1000, 0, &bw, 1); | ||
| r82xx_set_bandwidth(&tuner, bw, 0, &bw, 1); |
There was a problem hiding this comment.
i prefer this be a separate PR for both interface change and firmware change. Also change firmware needs to upload new firmware binary.
There was a problem hiding this comment.
The firmware binary is built during compilation of libsddc and included into it as a BLOB now. I think, it may be included into windows library this way too. I have not done this as I can't build Extio_SDDC...
Take a look at the original commit sequence and suggest changes (commits?), that may be included into a separate PR, please.
https://github.com/vladisslav2011/ExtIO_sddc/tree/linux_fixes
There was a problem hiding this comment.
it is streamlined to build on Linux easily. For Windows development, I normally don't recompile firmware for every build. :(
| $(MODULE).img: $(EXES) $(ELF2IMG) | ||
| $(ELF2IMG) -i $< -o $@ -v | ||
|
|
||
| $(MODULE).h: $(EXES) $(ELF2IMG) |
There was a problem hiding this comment.
what's this? why a image file use .h extension? hex?
There was a problem hiding this comment.
I've patched elf2img.c to generate .h file to include the firmware into the library as a BLOB. As we do not have persistent firmware storage (SPI flash), then it may be good to ship the firmware this way.
Suggest a better way of including the firmware into the library. Assembler .S file and incbin directive? Linker script?
Core/FX3Class.h
Outdated
| public: | ||
| virtual ~fx3class(void) {} | ||
| virtual bool Open(uint8_t* fw_data, uint32_t fw_size) = 0; | ||
| virtual bool Open(const uint8_t* fw_data, uint32_t fw_size) = 0; |
There was a problem hiding this comment.
this change can be one small PR.
|
Thanks for a review.
To be done later: |
|
thank you for effort. your plan looks great. |
cad6177 to
f7b7e36
Compare
|
1,4 and 5 (on top of latest PR) are done. |
Disable LPM while streaming to prevent dropouts.
Requires patched gr-osmosdr (gr3.7 only so far) https://github.com/vladisslav2011/gr-osmosdr/tree/ngrx No correct device listing and sample rates list. Gain selection is still a bit buggy. Fix test segfault. Do not delete fx3Handler as RadioHandlerClass::~RadioHandlerClass() have already taken care of it. Retry opening the USB device after download for 5 times As it may be slow to reenumerate on some platforms. Include the firmware into libsddc binary as a BLOB Make USB HS work at lower sampling rates/tuner at <7MHz BW Implement r82xx bandwidth selection Prevent freeze on close after live device disconnection Allow low sample rates Improve compatibility with intel hubs Restore attenuations after band switch
f7b7e36 to
caaa824
Compare
My original idea for Linux implementation is implementing a new r2iqCntrl class which doesn't do complex IQ converting. but it really depends on what we want to expose from libsddc. |
Requires patched gr-osmosdr (gr3.7 only so far)
https://github.com/vladisslav2011/gr-osmosdr/tree/ngrx
No correct device listing and sample rates list.
Gain selection is still a bit buggy.
Fix test segfault.
Do not delete fx3Handler as RadioHandlerClass::~RadioHandlerClass()
have already taken care of it.
Retry opening the USB device after download for 5 times
As it may be slow to reenumerate on some platforms.
Include the firmware into libsddc binary as a BLOB
Make USB HS work at lower sampling rates/tuner at <7MHz BW
Implement r82xx bandwidth selection
Prevent freeze on close after live device disconnection
Allow low sample rates
Improve compatibility with intel hubs
Restore attenuations after band switch
Merging this may close #201
Do not merge before testing under windows, please.
Hints and suggestions are welcome.