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

Error checking in populateBuffer! #11

Merged
merged 1 commit into from
May 26, 2023
Merged

Conversation

dd0
Copy link
Contributor

@dd0 dd0 commented May 24, 2023

Added an option to raise exceptions if received packet metadata indicates an error (see #10). This is disabled by default if the streamer is configured in continuous mode, to avoid breaking existing code that can ignore overflow errors and doesn't care about the exact number of samples received.

The exceptions are not exported to avoid adding generic names such as OverflowException to the global namespace.

Added an option to raise exceptions if received packet metadata
indicates an error (see JuliaTelecom#10). This is disabled by default if the
streamer is configured in continuous mode, to avoid breaking existing
code that can ignore overflow errors and doesn't care about the exact
number of samples received.

The exceptions are not exported to avoid adding generic names such as
OverflowException to the global namespace.
@RGerzaguet
Copy link
Member

Thanks for this initial PR. Looks very good to me.
2 small remarks

  • The timeout is not tested atm. Do you think we should also handle timing error or not ? Is there a reason the branching does not consider timeout error ?
  • I want to have a small test. Can you move the PR in a specific branch instead of main ?
    Thanks !!

@dd0
Copy link
Contributor Author

dd0 commented May 25, 2023

Hi,

The difference between a timeout and other errors is that a timeout only shows that the samples aren't available yet, but might arrive later, while the others all mean that something went wrong (samples dropped due to overflow, or a command / communication error). For example, if you schedule the recording for the future and immediately call recv!:

UHDBindings.restartStreamer(
    radio.rx,
    stream_mode = UHDBindings.LibUHD.UHD_STREAM_MODE_NUM_SAMPS_AND_MORE,
    num_samps = length(buffer),
    stream_now = false,
    full_sec_delay = 1,
    frac_sec_delay = 0)
UHDBindings.recv!(buffer, radio)

The first few calls to populateBuffer! will fail with a timeout because the SDR isn't streaming, and recv! will spin in the loop until it starts. I don't think that we should return an error here -- this looks like a reasonable way to use the API for delayed reception.

I created a branch with this commit in my fork, if that's what you meant: https://github.com/dd0/UHDBindings.jl/tree/error-checking
I can also change the PR to target another branch instead of main in the main repository, but (as far as I understand) that has to be an already-existing branch that a maintainer would have to create first.

@RGerzaguet
Copy link
Member

Ok thanks for the feedback. Let's merge and tag a new intermediate version :)

@RGerzaguet RGerzaguet merged commit c05290a into JuliaTelecom:master May 26, 2023
@RGerzaguet RGerzaguet mentioned this pull request May 26, 2023
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.

None yet

2 participants