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

Make it work under linux with gqrx/gr-osmosdr #223

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

Conversation

vladisslav2011
Copy link
Contributor

@vladisslav2011 vladisslav2011 commented May 9, 2022

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.

Copy link
Collaborator

@howard0su howard0su left a comment

Choose a reason for hiding this comment

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

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this get removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unable to make decimator work as expected, so I partially removed it. Offset tuning was removed as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oscar may help and he is expert on that DSP code.

Core/RadioHandler.cpp Outdated Show resolved Hide resolved
}

RadioHandlerClass::~RadioHandlerClass()
{
delete stateFineTune;
delete r2iqCntrl;
delete hardware;
delete fx3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this class owes the fx3 instance. it is allocated by someone else. better deallocate by the owner.

Copy link
Contributor Author

@vladisslav2011 vladisslav2011 May 9, 2022

Choose a reason for hiding this comment

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

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...

Copy link
Collaborator

Choose a reason for hiding this comment

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

it should not happen at least for Windows. Can you point me the crash stacks of two threads? I can help debug.

Core/RadioHandler.cpp Outdated Show resolved Hide resolved
Core/RadioHandler.cpp Outdated Show resolved Hide resolved
@@ -160,11 +152,13 @@ template<typename T> class ringbuffer : public ringbufferbase {

T* peekWritePtr(int offset)
{
std::unique_lock<std::mutex> lk(mutex);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why? atomic read doesn't need mutex lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. This may be removed. I have added mutex locking to every method trying to make it output continous stream of samples.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you repro the issue via a unit test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@vladisslav2011 vladisslav2011 May 10, 2022

Choose a reason for hiding this comment

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

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.cpp Outdated Show resolved Hide resolved
void TurnOff(void);
bool IsOn(void);
void TurnOn() override;
void TurnOff(void) override;
Copy link
Collaborator

Choose a reason for hiding this comment

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

please keep consistent. I prefer not put void here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

r82xx_init(&tuner);
r82xx_set_bandwidth(&tuner, 8*1000*1000, 0, &bw, 1);
r82xx_set_bandwidth(&tuner, bw, 0, &bw, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

i prefer this be a separate PR for both interface change and firmware change. Also change firmware needs to upload new firmware binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

it is streamlined to build on Linux easily. For Windows development, I normally don't recompile firmware for every build. :(

@@ -107,4 +108,7 @@ $(ELF2IMG) : $(FX3FWROOT)/util/elf2img/elf2img.c
$(MODULE).img: $(EXES) $(ELF2IMG)
$(ELF2IMG) -i $< -o $@ -v

$(MODULE).h: $(EXES) $(ELF2IMG)
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's this? why a image file use .h extension? hex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
@@ -20,7 +20,7 @@ class fx3class
{
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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this change can be one small PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

@vladisslav2011
Copy link
Contributor Author

vladisslav2011 commented May 10, 2022

Thanks for a review.
Changes plan:

  1. Split FX3Class.h into a separate PR
  2. Split ringbuffer mutex addition into a separate PR
  3. Split r828d bandwidth setting (firmware and host) into a separate PR
  4. Split Intel HUB freeze fix (firmware) into a separate PR
  5. Rebase this PR with latest changes on top of the sequence and request a review.

To be done later:
6. Do not waste CPU time copying buffers when not using the decimator.
7. Make the decimator work under linux. And maybe try switch to FIR as it should be more efficient, than FFT.
8. Switch from plain pointers to std::shared_ptr.

@howard0su
Copy link
Collaborator

thank you for effort. your plan looks great.

@vladisslav2011 vladisslav2011 force-pushed the linux_fixes_upstream branch 2 times, most recently from cad6177 to f7b7e36 Compare May 10, 2022 08:47
@vladisslav2011
Copy link
Contributor Author

1,4 and 5 (on top of latest PR) are done.
2 is not required without arbitrary sampling rate implementation as it fixes crash when the sampling rate is changed while streaming.
3 would be hard to do while keeping compatibility with downconverter as changing the bandwidth would change center frequency offset too. It was done to make the device work at HS speed to reduce L-band interference from USB SS HUBS.

vladisslav2011 and others added 2 commits May 10, 2022 19:13
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
@howard0su
Copy link
Collaborator

1,4 and 5 (on top of latest PR) are done. 2 is not required without arbitrary sampling rate implementation as it fixes crash when the sampling rate is changed while streaming. 3 would be hard to do while keeping compatibility with downconverter as changing the bandwidth would change center frequency offset too. It was done to make the device work at HS speed to reduce L-band interference from USB SS HUBS.

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.

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

Successfully merging this pull request may close these issues.

RX888 Not working on Linux
2 participants