host: Implement n-times repeated transmission in tx_samples_from_file example #679
Conversation
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
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
| } | ||
| FLUSH_SCREEN(); | ||
| } while (0); | ||
| int y, x; |
There was a problem hiding this comment.
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
yvariable 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.
|
I have read the CLA Document and I hereby sign the CLA |
nzqo
changed the title
|
recheck |
I only see CI errors that seem related to some tokens, anything that I can do here? 😁 |
|
recheck |
Nah the problem is on our end, we changed some github settings and now are chasing down things like this. |
|
recheck |
|
(sorry I'm using your PR for testing purposes) |
Feel free, I dont mind 😁 |
|
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))); |
There was a problem hiding this comment.
moving this sleep here modify the behavior: it waits before first execution and emission. This new behavior seems not expected to me.
There was a problem hiding this comment.
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}) | |||
There was a problem hiding this comment.
Can you add a comment to describe the goal of including directoy "SYSTEM". May include it in a makefile line dedicated to it.
| @@ -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)) { | |||
There was a problem hiding this comment.
Combinatory condition should be a 'or' as both options shall not be used together
- Fixed sigint handler condition - Restored previous delay behavior - Added explanatory command for boost system header inclusion
Pull Request Details
Description
These changes allow to use the
tx_samples_from_fileexample with a specified number of repetitions using then-repeatoption.I didn't know whether breaking compatibility was okay, so I did not. Instead, the
--repeatoption 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 appropriatesig_atomic_tflag 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_namewasnt manually set in CMake, which caused a warning in newer CMake versions due to breaking changesWhich devices/areas does this affect?
Affects host uhd lib (no functional changes) and the
tx_samples_from_fileexample (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 added tests to cover my changes, andall previous tests pass.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.