graph-prototype icon indicating copy to clipboard operation
graph-prototype copied to clipboard

Add blocks: Multiply, Divide, Add, Subtract

Open eltos opened this issue 1 year ago • 8 comments

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.

  • [x] Implement the following blocks from https://github.com/fair-acc/gnuradio4/issues/161 which take N inputs and combine them into a single output using one of four operations
    • Add
    • Divide
    • Multiply
    • Subtract
  • [x] Test and debug locally
  • [x] Add unit test

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 avatar Aug 27 '24 15:08 eltos

@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...

RalphSteinhagen avatar Sep 25 '24 06:09 RalphSteinhagen

@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

eltos avatar Sep 25 '24 08:09 eltos

@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.

RalphSteinhagen avatar Sep 25 '24 12:09 RalphSteinhagen

@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

eltos avatar Sep 26 '24 11:09 eltos

@all-contributors please add @eltos for plugin

RalphSteinhagen avatar Sep 26 '24 14:09 RalphSteinhagen

@RalphSteinhagen

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

allcontributors[bot] avatar Sep 26 '24 14:09 allcontributors[bot]

@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.

RalphSteinhagen avatar Sep 26 '24 17:09 RalphSteinhagen

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.

eltos avatar Sep 30 '24 09:09 eltos

@eltos As already discussed this unfortunately was not merged in a timely manner on our side and now there were some API changes that had to be followed up. I already did them, but it seems I cannot update your PR branch. Could you either allow gnuradio4 maintainers to push to your branch (see here) or I would close this one and create a new PR.

wirew0rm avatar Oct 30 '24 15:10 wirew0rm

@wirew0rm you should be able to push now, I enabled the setting.

eltos avatar Oct 30 '24 15:10 eltos

Merged as the remaining CI failures were unrelated and will be solved with #455

Thank you again for giving this a go already at this early stage and your patience and feedback!

wirew0rm avatar Oct 31 '24 10:10 wirew0rm

@drslebedev with this merged, can you tick the corresponding blocks in #161 please?

eltos avatar Nov 22 '24 09:11 eltos