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

Add blocks: Multiply, Divide, Add, Subtract #395

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

eltos
Copy link

@eltos eltos commented Aug 27, 2024

To learn how to implement blocks in gr4 I want to implement and contribute a simple math block as part of the Block Tutorial at European GNU Radio Days 2024. This PR is a work-in-progress state of my efforts.

Usage example:

gr::Graph graph{};
// source_1 counting 1,2,3,4,...,10
auto& source_1 = graph.emplaceBlock<gr::testing::CountingSource<float>>({{"n_samples_max", 10U}});
// source_2 counting 6,7,8,9,...,15
auto& source_2 = graph.emplaceBlock<gr::testing::CountingSource<float>>({{"n_samples_max", 10U}});
source_2.default_value = 5.0f;
// Add block with two inputs
auto& adder = graph.emplaceBlock<gr::math::Add<float>>({{"n_inputs", 2U}});
// Null sink
auto& sink = graph.emplaceBlock<gr::testing::CountingSink<float>>();

// Connections (note how the two inputs of the adder block are addressed)
auto connected = true;
connected &= graph.connect<"out">(source_1).to<"in", 0>(adder) == gr::ConnectionResult::SUCCESS;
connected &= graph.connect<"out">(source_2).to<"in", 1>(adder) == gr::ConnectionResult::SUCCESS;
connected &= graph.connect<"out">(adder).to<"in">(sink) == gr::ConnectionResult::SUCCESS;
if (!connected) { fmt::print("Error: Failed to connect flowgraph!\n"); return; }

// Execute flow graph
gr::scheduler::Simple<> scheduler{std::move(graph)};
scheduler.runAndWait();

Feedback:

I found it difficult to implement the block with a dynamic number of ports since I didn't find documentation on this, nor did I find a description on how to connect the ports in the graph (tried "in0", "in#0" etc. first).
I finally got it, but since this is a very general concept, it would be beneficial to provide the mechanics of "n_inputs" settings etc. in the base block and enable something like PortInMulti<T, Limits<1, 32>> in; for ease of use.
Also, having more example flowgraphs like the one above would be helpful for newcomers.

@eltos eltos marked this pull request as draft August 27, 2024 15:03
@eltos eltos changed the title WIP: Add blocks: Multiply, Divide, Add, Subtract Add blocks: Multiply, Divide, Add, Subtract Aug 30, 2024
@eltos eltos marked this pull request as ready for review August 30, 2024 15:03
@RalphSteinhagen
Copy link
Member

@eltos thanks for your PR. Much appreciated. In order to be able to review this, this needs to have the corresponding unit-tests and QA/CI-pipeline passing. If you could have a look...

@eltos
Copy link
Author

eltos commented Sep 25, 2024

@RalphSteinhagen the CI failure is not related to the PR:

The following tests FAILED:
	 25 - qa_FilterTool (Subprocess aborted)
	 32 - qa_StreamToDataSet (Failed)
Errors while running CTest

I've merged the main branch, hope that helps

@RalphSteinhagen
Copy link
Member

@eltos, the main thing is that you added code without unit tests. @wirew0rm is working on the correct accounting, but all code (especially when no hardware is involved) should aim for 100% coverage. Your IDE/sonartype/... are your friends.

@eltos
Copy link
Author

eltos commented Sep 26, 2024

@eltos, the main thing is that you added code without unit tests.

True. Sadly, this was not covered in the developer tutorials during the workshop.
It seems that no generic way of testing blocks is foreseen, or I was not able to find it.

Anyway, I have now added a test_block function that tests the 4 blocks of this PR by feeding in data and comparing the result.
Since this is a somewhat universal concept common to any block, you could think about generalizing it. If it is included in the tutorials and documentation, future contributors may benefit from not having to compile the full testing logic, but just define a set of test input samples and expected output samples.

"Add"_test = []<typename T>(const T&) {
    test_block<T, Add<T>>({
        .inputs = {{1, 2,  3, T( 4.2)},
                   {5, 6,  7, T( 8.3)}},
        .output = { 6, 8, 10, T(12.5)}
    });
} | kArithmeticTypes;

The test passes for me locally:
grafik

Copy link
Member

@RalphSteinhagen RalphSteinhagen left a comment

Choose a reason for hiding this comment

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

@eltos thanks a lot for your commit and patience/compliance w.r.t. the QA compliances. This goes a long way and helps keeping the code maintainable! 👍

I like the tests you write and should be sufficient. Will just wait that the first CI test pass and then merge it as is.

Thanks again and welcome to the GR4 maintainer team. 😁 🎉

@RalphSteinhagen
Copy link
Member

@all-contributors please add @eltos for plugin

Copy link
Contributor

@RalphSteinhagen

I've put up a pull request to add @eltos! 🎉

@RalphSteinhagen
Copy link
Member

@eltos, your unit test doesn't seem to be portable (Emscripten, probably 32bit vs. 64bit). Could you have a look and fix this? Thanks in advance.

@eltos
Copy link
Author

eltos commented Sep 30, 2024

I can't see where the qa_math unit test fails? I am not familiar with emscripten, but what I can see from the emcc job's logs is a kill with error 137 while building the scheduler (before even building or executing qa_math).
137 indicates out-of-memory.
Looks like the 16GB provided by GitHub runners are insufficient.

Killed
gmake[2]: *** [core/benchmarks/CMakeFiles/bm_Scheduler.dir/build.make:77: core/benchmarks/CMakeFiles/bm_Scheduler.dir/bm_Scheduler.cpp.o] Error 137

Other than that there are some warnings on implicit conversion in some of the dependencies (pmt, ut, fmt) which are not related to this PR, and there is a warning on the usage of ALLOW_MEMORY_GROWTH.

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.

2 participants