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

host: Implement n-times repeated transmission in tx_samples_from_file example #679

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

Conversation

nzqo
Copy link

@nzqo nzqo commented May 15, 2023

Pull Request Details

Description

These changes allow to use the tx_samples_from_file example with a specified number of repetitions using the n-repeat option.

I didn't know whether breaking compatibility was okay, so I did not. Instead, the --repeat option overwrites the new flag and still causes transmission until a SIGINT is received. I did, however, change that signal handling to be conformal by using the appropriate sig_atomic_t flag type.

On the way, I made some minor formatting change, got rid of a few annoying compiler warnings and improved some CMake stuff. Specifically:

  • install_name wasnt manually set in CMake, which caused a warning in newer CMake versions due to breaking changes
  • Version range in DPDK find package module wasnt handled properly. Another warning that was easy to fix
  • Use of sprintf which I simply substituted with snprintf
  • A missing virtual destructor which could potentially have lead to memory issues

Which devices/areas does this affect?

Affects host uhd lib (no functional changes) and the tx_samples_from_file example (convenience feature).

Testing Done

Present tests pass. I dont think there are tests for the examples though. Since the changes are mostly
minor I dont think new tests are required.

As for the example itself, I ran it with a few different options (repeat, n-repeat, both) to confirm that it doesnt crash or anything.

Checklist

  • I have read the CONTRIBUTING document.
  • 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, ...)

Additional notes

It might make sense to bring in codestyle for CMake files as well (and e.g. enforce it using cmake-format).
Also, I didnt want to touch it, but some of the boost filesystem stuff is deprecated in favor of C++17's filesystem. I was considering abstracting the relevant functions away and switching between the two depending on the supported standard of the local compiler, but that was too much hassle to do for now.

nzqo added 4 commits May 15, 2023 10:22
Newer versions of cmake do not set install_name, so we need to do it
ourselves.
FindDPDK module did not properly handle version range before, this
change fixes this.
Also made boost a system dependency to suppress compiler warnings from
it.
Some minor formatting, spaces etc
This change fixes a few annoying compiler warnings. Mostly, those
are related to unused variables which are simply suppressed.
Also fixes a missing virtual destructor which could have resulted
in memory issues.
Implement n-repeat option in tx_samples_from_file example to repeat a
transmission for a specified amount of times
@github-actions
Copy link

github-actions bot commented May 15, 2023

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

}
FLUSH_SCREEN();
} while (0);
int y, x;
Copy link
Author

Choose a reason for hiding this comment

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

It would be nice if someone could take a closer look at this part. To me, this looked a bit off:

  • It seems like this was copied out of some ancient MACRO because of the do{} while(0) usage
  • The y variable is unused but was incremented. I am not sure what the original intent here was, so this might still not do what it is supposed to.

@nzqo
Copy link
Author

nzqo commented May 15, 2023

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

@nzqo nzqo changed the title Implement n-times repeated transmission in tx_samples_from_file example host: Implement n-times repeated transmission in tx_samples_from_file example May 15, 2023
@mbr0wn
Copy link
Contributor

mbr0wn commented Jul 11, 2023

recheck

@nzqo
Copy link
Author

nzqo commented Jul 13, 2023

recheck

I only see CI errors that seem related to some tokens, anything that I can do here? 😁

@mbr0wn
Copy link
Contributor

mbr0wn commented Jul 13, 2023

recheck

@mbr0wn
Copy link
Contributor

mbr0wn commented Jul 13, 2023

recheck

I only see CI errors that seem related to some tokens, anything that I can do here? grin

Nah the problem is on our end, we changed some github settings and now are chasing down things like this.

@mbr0wn
Copy link
Contributor

mbr0wn commented Jul 13, 2023

recheck

@mbr0wn
Copy link
Contributor

mbr0wn commented Jul 13, 2023

(sorry I'm using your PR for testing purposes)

@nzqo
Copy link
Author

nzqo commented Jul 13, 2023

(sorry I'm using your PR for testing purposes)

Feel free, I dont mind 😁

@mbr0wn
Copy link
Contributor

mbr0wn commented Jul 13, 2023

Some progress: https://github.com/EttusResearch/uhd/actions/runs/5541486415/jobs/10114946490#step:2:16

...it's just not updating here yet.

break;

if(repeat and (delay > 0.0)) {
std::this_thread::sleep_for(std::chrono::milliseconds(int64_t(delay * 1000)));
Copy link

@JohnJohn06 JohnJohn06 Oct 17, 2023

Choose a reason for hiding this comment

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

moving this sleep here modify the behavior: it waits before first execution and emission. This new behavior seems not expected to me.

Copy link
Author

Choose a reason for hiding this comment

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

Moved it back and restored the old behavior.

BTW: I made it as to not break backwards compatibility (still have the --repeat) option. It could be merged by allowing n_repeat=-1 as substitute for repeat, i.e. repeating indefinitely. Which version would you prefer?

@@ -298,15 +314,17 @@ set(UHD_BOOST_REQUIRED_COMPONENTS

include(UHDBoost)

include_directories(${Boost_INCLUDE_DIRS})
include_directories(SYSTEM ${Boost_INCLUDE_DIRS})
Copy link

@JohnJohn06 JohnJohn06 Oct 17, 2023

Choose a reason for hiding this comment

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

Can you add a comment to describe the goal of including directoy "SYSTEM". May include it in a makefile line dedicated to it.

Copy link
Author

Choose a reason for hiding this comment

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

Added a comment here

@@ -201,7 +207,7 @@ int UHD_SAFE_MAIN(int argc, char* argv[])
}

// set sigint if user wants to receive
if (repeat) {
if (repeat and (n_repeats > 0)) {
Copy link

@JohnJohn06 JohnJohn06 Oct 17, 2023

Choose a reason for hiding this comment

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

Combinatory condition should be a 'or' as both options shall not be used together

Copy link
Author

Choose a reason for hiding this comment

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

fixed

nzqo and others added 2 commits October 17, 2023 13:30
- Fixed sigint handler condition
- Restored previous delay behavior
- Added explanatory command for boost system header inclusion
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.

3 participants