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

nzqo · 2023-05-15T08:57:41Z

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.

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 · 2023-05-15T08:57:57Z

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

nzqo · 2023-05-15T08:59:11Z

host/utils/latency/lib/Responder.cpp

-        }
-        FLUSH_SCREEN();
-    } while (0);
+    int y, x;


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 · 2023-05-15T09:05:26Z

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

mbr0wn · 2023-07-11T10:55:57Z

recheck

nzqo · 2023-07-13T08:24:32Z

recheck

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

mbr0wn · 2023-07-13T09:17:10Z

recheck

mbr0wn · 2023-07-13T09:17:50Z

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 · 2023-07-13T09:34:02Z

recheck

mbr0wn · 2023-07-13T09:34:12Z

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

nzqo · 2023-07-13T09:34:55Z

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

Feel free, I dont mind 😁

mbr0wn · 2023-07-13T09:37:01Z

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

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

JohnJohn06 · 2023-10-17T09:19:00Z

host/examples/tx_samples_from_file.cpp

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


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

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?

JohnJohn06 · 2023-10-17T09:36:44Z

host/CMakeLists.txt

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

 include(UHDBoost)

-include_directories(${Boost_INCLUDE_DIRS})
+include_directories(SYSTEM ${Boost_INCLUDE_DIRS})


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

Added a comment here

JohnJohn06 · 2023-10-17T09:54:39Z

host/examples/tx_samples_from_file.cpp

@@ -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)) {


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

mbr0wn · 2024-11-12T08:54:48Z

Hey @nzqo, many thanks for the submission. After a lengthy discussion, we decided to not merge this change request, because we are trying to keep our examples lean and functional, by which we mean they are not supposed to become general utilities, but rather, should exemplify a small number of features clearly and concisely.

Now, I admit we haven't always done a good job with that. Some examples have suffered from feature creep, and not all are perfect, well-documented shiny beacons of example-ness. But we're trying to improve that, and this change doesn't work in the right direction.

Again, many thanks for your submission.

nzqo added 4 commits May 15, 2023 10:22

BuildSystem: Minor Formatting

f9dd009

Some minor formatting, spaces etc

Host: Fix a few compiler warnings

49e555e

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.

Example: Implement n-times repeated transmission

a287864

Implement n-repeat option in tx_samples_from_file example to repeat a transmission for a specified amount of times

nzqo commented May 15, 2023

View reviewed changes

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

Merge remote-tracking branch 'upstream/master'

ab2b349

JohnJohn06 reviewed Oct 17, 2023

View reviewed changes

nzqo and others added 2 commits October 17, 2023 13:30

Merge branch 'EttusResearch:master' into master

c917f69

MR Review updates

5e69181

- Fixed sigint handler condition - Restored previous delay behavior - Added explanatory command for boost system header inclusion

mbr0wn closed this Nov 12, 2024

github-actions bot locked and limited conversation to collaborators Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nzqo commented May 15, 2023 •

edited

Loading

github-actions bot commented May 15, 2023 •

edited

Loading

nzqo May 15, 2023

nzqo commented May 15, 2023

mbr0wn commented Jul 11, 2023

nzqo commented Jul 13, 2023

mbr0wn commented Jul 13, 2023

mbr0wn commented Jul 13, 2023

mbr0wn commented Jul 13, 2023

mbr0wn commented Jul 13, 2023

nzqo commented Jul 13, 2023

mbr0wn commented Jul 13, 2023

JohnJohn06 Oct 17, 2023 •

edited

Loading

nzqo Oct 17, 2023

JohnJohn06 Oct 17, 2023 •

edited

Loading

nzqo Oct 17, 2023

JohnJohn06 Oct 17, 2023 •

edited

Loading

nzqo Oct 17, 2023

mbr0wn commented Nov 12, 2024

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

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

Conversation

nzqo commented May 15, 2023 • edited Loading

Pull Request Details

Description

Which devices/areas does this affect?

Testing Done

Checklist

Additional notes

github-actions bot commented May 15, 2023 • edited Loading

nzqo May 15, 2023

Choose a reason for hiding this comment

nzqo commented May 15, 2023

mbr0wn commented Jul 11, 2023

nzqo commented Jul 13, 2023

mbr0wn commented Jul 13, 2023

mbr0wn commented Jul 13, 2023

mbr0wn commented Jul 13, 2023

mbr0wn commented Jul 13, 2023

nzqo commented Jul 13, 2023

mbr0wn commented Jul 13, 2023

JohnJohn06 Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

nzqo Oct 17, 2023

Choose a reason for hiding this comment

JohnJohn06 Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

nzqo Oct 17, 2023

Choose a reason for hiding this comment

JohnJohn06 Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

nzqo Oct 17, 2023

Choose a reason for hiding this comment

mbr0wn commented Nov 12, 2024

nzqo commented May 15, 2023 •

edited

Loading

github-actions bot commented May 15, 2023 •

edited

Loading

JohnJohn06 Oct 17, 2023 •

edited

Loading

JohnJohn06 Oct 17, 2023 •

edited

Loading

JohnJohn06 Oct 17, 2023 •

edited

Loading