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.

Sign up for GitHub

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

Jump to bottom

Frank/datasink metadata #283

Merged
merged 7 commits into from
Mar 7, 2024
Merged

Frank/datasink metadata #283

merged 7 commits into from
Mar 7, 2024

Conversation

frankosterfeld
Copy link
Contributor

@frankosterfeld frankosterfeld commented Feb 22, 2024 •
edited
Loading

Fix issues in DataSink:

Make sure lookup by signal_name is thread-safe and is not accessing the
signal_name member directly, which might be updated in another thread.

Simplify and fix the signal metadata propagation to work with the automatic forwarding of source properties matching default tags.

Fix the metadata propagation for GRC-loaded graphs.

Needed for fair-acc/opendigitizer#159 and fair-acc/opendigitizer#160

@frankosterfeld frankosterfeld force-pushed the frank/datasink-metadata branch from cd6afff to d773abc Compare February 22, 2024 23:19
@frankosterfeld frankosterfeld force-pushed the frank/datasink-metadata branch from d773abc to 9c0bb0a Compare February 23, 2024 10:59
@frankosterfeld frankosterfeld force-pushed the frank/datasink-metadata branch from 9c0bb0a to 8179c30 Compare February 23, 2024 11:55
@frankosterfeld frankosterfeld force-pushed the frank/datasink-metadata branch from 8179c30 to 4a9abfa Compare February 23, 2024 12:43
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 6, 2024 15:24 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 6, 2024 15:25 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 6, 2024 15:25 — with GitHub Actions Inactive
@frankosterfeld
Do not register block at init time
357666a 
Revert the renaming of Block::init() to Block::performInit().

Instead of registering the DataSink at init time, do so in start(),
and unregister in stop().

This needs changes in the tests, as a poller/callback can now only
be registered while the execution is already running, the
pollers/callbacks might not get all the data from the start.
Add a initial delay (new block testing::Delay) to work around that.
@frankosterfeld frankosterfeld force-pushed the frank/datasink-metadata branch from 8c4cc79 to 357666a Compare March 7, 2024 11:23
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 11:24 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 11:24 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 11:24 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 11:24 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 11:24 — with GitHub Actions Inactive
RalphSteinhagen
RalphSteinhagen requested changes Mar 7, 2024
View reviewed changes
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.

@frankosterfeld , thanks for the update. 👍

I like the std::optional<U> getProperty(const property_map &m, std::string_view key);. We should (separate PR) elevate this to using std::expected<..> and make this a core function or perhaps for the pmt-library itself as this is a common use case that I also saw @ivan-cukic introduced for the message handling.

I also like the GRC-based loader-based unit-test in qa_Settings. Why is this disabled for Emscripten?
As discussed in the GR Architecture channel, this grc-file-based definition is good both for unit-tests as well as basic-usage documentation. We had a quick iteration over here, and maybe we will add another optional using ... = fixed_string("grc definition"); to the blocks to allow for documenting these (at least the basic) use cases. Unit-tests and documentation aside, this could also be used on the GR website to launch a WASM GR UI for the block documentation where users could directly play/experiment with a give block.

Regarding the modifications to core library functions in Scheduler.hpp and Block.hpp (comments only), unless these changes address specific bugs or are critically needed, I suggest reverting them due to concurrent modifications being made in other PRs.

Your contributions are valuable, and with these minor adjustments, this PR could be merged. Thanks in advance.

core/include/gnuradio-4.0/Block.hpp Outdated Show resolved Hide resolved
core/include/gnuradio-4.0/Block.hpp Outdated Show resolved Hide resolved
core/include/gnuradio-4.0/Scheduler.hpp Outdated Show resolved Hide resolved
core/test/qa_Settings.cpp
#ifndef __EMSCRIPTEN__
// TODO enable this when load_grc works in emscripten (not relying on plugins here)
"Property auto-forwarding with GRC-loaded graph"_test = [&] {
constexpr std::string_view grc = R"(
Copy link
Member

Choose a reason for hiding this comment

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

I like this, this is useful. 👍

core/test/qa_Settings.cpp Show resolved Hide resolved
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 12:56 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 12:56 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 12:56 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 12:56 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 12:56 — with GitHub Actions Inactive
RalphSteinhagen
RalphSteinhagen approved these changes Mar 7, 2024
View reviewed changes
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.

@frankosterfeld thanks for the changes. Will squash-merge it once the CI passes.

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Mar 7, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
67.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 15:46 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 15:46 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 15:46 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 15:46 — with GitHub Actions Inactive
@frankosterfeld frankosterfeld temporarily deployed to configure coverage March 7, 2024 15:46 — with GitHub Actions Inactive
@RalphSteinhagen RalphSteinhagen merged commit 439894a into main Mar 7, 2024
6 of 8 checks passed
@RalphSteinhagen RalphSteinhagen deleted the frank/datasink-metadata branch March 7, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalphSteinhagen RalphSteinhagen RalphSteinhagen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@frankosterfeld @RalphSteinhagen