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

Refactor UI blocks and use multithreaded scheduler #149

Merged
merged 8 commits into from
Feb 6, 2024

Conversation

frankosterfeld
Copy link
Contributor

@frankosterfeld frankosterfeld commented Jan 31, 2024

Convert the UI blocks RemoteDataSource, SineSource and Arithmetic into "plain" GR blocks that could be moved out of opendigitizer later. This temporarily removes the the remote flowgraph handling for RemoteSource, which will be re-added later.

Also use a multithreaded scheduler, and stop and delete the old graph/scheduler once a new one is set (this required some fixes in RemoteSource and SineSource to avoid freezes/crashes on destruction).

Bump opencmw-cpp to include some fixes for RestClientEmscripten (GET/SUBSCRIBE).

@frankosterfeld frankosterfeld linked an issue Jan 31, 2024 that may be closed by this pull request
Fixes issues with emscripten GET/SET, i.e. initial dashboard retrieval,
and subscribe/unsubscribe (query parameters got lost).
Remove special treatment for remote sources, make it a regular GR
block.

TODO:
 - readd remote flowgraph handling
 - readd adding new sources from the UI (wait for messages?)
RestClient's tasks continue running when the RestClient is destroyed,
the callback then accessed the already deleted "this". Move the shared
data to a shared_ptr, to ensure it's still valid when appending data to
it.
Make sure that destroyed while waiting in processOne(), there
isn't a deadlock where both mutex destruction and processOne
block each other.
@frankosterfeld frankosterfeld changed the title WIP Make remote sources a regular GR block Refactor UI blocks and use multithreaded scheduler Feb 6, 2024
@frankosterfeld frankosterfeld marked this pull request as ready for review February 6, 2024 09:08
struct MathNode : public gr::Block<MathNode<T>> {
gr::PortIn<T> in1{};
gr::PortIn<T> in2{};
struct Arithmetic : public gr::Block<Arithmetic<T>> {
Copy link
Member

Choose a reason for hiding this comment

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

generally OK for this PR but we should move all of these isolated block implementation to the GR 4.0 repo and also make them conform and check the corresponding items in summary issue EPIC: List of GR 3.10 blocks & Others to be ported to GR 4.0

@@ -40,31 +39,35 @@ struct SineSource : public gr::Block<SineSource<T>, gr::BlockingIO<true>> {
}

~SineSource() {
std::unique_lock guard(mutex);
Copy link
Member

Choose a reason for hiding this comment

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

Should be replaced and synchronised with EPIC: List of GR 3.10 blocks & Others to be ported to GR 4.0. Also there is already a sine and cosine source in GR 4.0.

@drslebedev maybe you could comment/document the usage and check this if this conforms to the functionality in GR 3.X.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the example of SignalGenerator which can generate sine wave (this is the default signal type), note that ClockSource is required sa input to SignalGenerator:

constexpr std::uint32_t n_samples   = 200;
constexpr float         sample_rate = 1000.f;
Graph                   testGraph;
auto                   &clockSrc  = testGraph.emplaceBlock<gr::basic::ClockSource<float>>({ { "sample_rate", sample_rate }, { "n_samples_max", n_samples }, { "name", "ClockSource" } });
auto                   &signalGen = testGraph.emplaceBlock<SignalGenerator<float>>({ { "sample_rate", sample_rate }, { "name", "SignalGenerator" } });

testGraph.connect<"out">(clockSrc).to<"in">(signalGen);
testGraph.connect<"out">(signalGen).to<"in">(sink);

scheduler::Simple sched{ std::move(testGraph) };
sched.runAndWait();

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.

Looks OK for me from a code-only perspective. Many of the non-UI blocks (and those not dependent on OpenCMW) should become part of the GR 4.0 block-lib. This reduces the duplicate work/documentation/testing. See the EPIC: List of GR 3.10 blocks & Others to be ported to GR 4.0 for details.

Can IMO be merged if @drslebedev and/or @wirew0rm had another chance to functionally cross-check.

@drslebedev drslebedev self-requested a review February 6, 2024 10:32
Copy link
Contributor

@drslebedev drslebedev left a comment

Choose a reason for hiding this comment

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

I added an example of Sine wave generator in the comment to this PR.
The rest looks fine to me and PR can be merged, just keeping in mind in the next iteration to use blocks from GR4.0 where possible.

@RalphSteinhagen RalphSteinhagen merged commit 87a40ca into main Feb 6, 2024
8 of 9 checks passed
@RalphSteinhagen RalphSteinhagen deleted the frank/remotesource-refactoring branch February 6, 2024 10:36
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.

[5SP,5SP] UI: Refactor flow-graph block handling
4 participants