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 setting start_time having no effect in Python bindings #665

Conversation

zolveuran
Copy link
Contributor

@zolveuran zolveuran commented Mar 14, 2023

Pull Request Details

Description

Transmission starts immediately even though a start_time is provided when using MultiUSRP.send_waveform() in the Python bindings. Caused by metadata.has_time_spec not being set to True when metadata.time_spec is set.

Suspect the same issue is present for recv_num_samps, where stream_cmd.stream_now = False is missing when stream_cmd.time_spec = start_time.

Related Issue

Fixes #664

Which devices/areas does this affect?

Tested on USRP 2954.

Testing Done

Tested by setting time_spec = usrp.get_time_now().get_real_secs() + 10 and pass it to MultiUSRP.send_waveform().
Am currently not able to test recv_num_samps() due to no physical access to the hardware at the moment, but according to the documentation stream_cmd.stream_now should be set to False. Would be great if someone could give that a quick test as well.

Checklist

  • [x ] I have read the CONTRIBUTING document.
  • [x ] My code follows the code style of this project. See CODING.md.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes, and all previous tests pass.
  • I have checked all compat numbers if they need updating (FPGA compat,
    MPM compat, noc_shell, specific RFNoC block, ...)

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@zolveuran
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@zolveuran
Copy link
Contributor Author

recheck

@mbr0wn
Copy link
Contributor

mbr0wn commented Jul 10, 2023

Many thanks for the find! Can you please use an email address that matches what you have in your account?

@mbr0wn
Copy link
Contributor

mbr0wn commented Jul 13, 2023

recheck

1 similar comment
@zolveuran
Copy link
Contributor Author

recheck

…pec and stream_now when using Python bindings.
@zolveuran zolveuran force-pushed the uran/fix_python_start_transmit_start_time branch from 280ae01 to e0c5b6e Compare July 13, 2023 11:16
@zolveuran
Copy link
Contributor Author

recheck

@zolveuran
Copy link
Contributor Author

@mbr0wn I tried to add the other e-mail address as a secondary, but that didn't work. Tried to amend the original commit to update the e-mail address, but still no luck. Maybe I'm not able to trigger a recheck? Can you have another try?

Would also like to comment that the CLA is creating a somewhat of a barrier towards contributing - even for minor issues. Also, as the CONTRIBUTING.md document points out the CLA should only be required for "non-trivial contribution". I'd say this commit is fairly trivial and should not require the CLA to be signed.

Copy link
Contributor

@mbr0wn mbr0wn left a comment

Choose a reason for hiding this comment

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

Many thanks. We can figure out what's wrong with the CLA checker.

@mbr0wn mbr0wn added the Fixed Internally This issue has been fixed but needs to clear our internal release pipeline. label Nov 29, 2023
@mbr0wn
Copy link
Contributor

mbr0wn commented Nov 29, 2023

This is percolating our internal CI and review pipelines. It might get closed before the changeset goes live on public master.

@@ -81,6 +81,7 @@ def _start_stream(streamer):
if not stream_cmd.stream_now:
if start_time is not None:
stream_cmd.time_spec = start_time
stream_cmd.stream_now = False
Copy link
Contributor

@mbr0wn mbr0wn Dec 4, 2023

Choose a reason for hiding this comment

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

This line is not necessary (see the if clause 3 lines above, it's a requirement for coming here in the first place).

@atomita-ni atomita-ni closed this Jan 25, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed Internally This issue has been fixed but needs to clear our internal release pipeline.
Projects
None yet
2 participants