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

Fix RX streaming space required calculation #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

josh-sw-is-fun
Copy link

The calculation to compute the space required to store the users samples is
double what it should be.

An example of how this can manifest from the users perspective, the Sidekiq SDR
reads 2036 samples of type int16_t/float, the internal Soapy Sidekiq buffer
will be resized to twice what is needed, samples are written to the first half
of the buffer, the 2nd half is all zeros. If you dump the samples received
from SoapySidekiq::readStream, you would see a pattern of 2036 samples
followed by 2036 samples consisting of zeros.

Instead, the space_req should be in terms of the number of 'samples' which
are I & Q samples.

There was a bit of confusion on my part as to what a sample represented. A
sample represents a single I or Q value. Something like a
std::complex<float>, which I initially assumed was a 'sample' is instead
referred to as an elem or elements in this code. Not a criticism but it
seems like different code bases represent a 'sample' differently.

This change was tested with:

  • Sidekiq Stretch M.2-2280 (rev B), FPGA v3.15.1
  • Sidekiq SDK v4.17.4
  • Stream types int16_t (Soapy CS16) and float (Soapy CF32)

There are 3 additional unrelated changes in this commit:

  • Replace magic number 2 with elementsPerSample in memcpy buffer size
    calculation.
  • Use microsecond unit name (us) when reporting timeout rather than reporting
    as milliseconds unit name (ms).
  • GitHub recommended adding a new line at the end of the file

The calculation to compute the space required to store the users samples is
double what it should be.
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.

1 participant