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

Improvements to the plugin system with examples #88

Merged
merged 7 commits into from
Jun 16, 2023

Conversation

ivan-cukic
Copy link
Contributor

No description provided.

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 good!

Some more structural comments on the dynamic ports and hire functions. The main things that should IMO be addressed in this PR:

  • finding a good global naming and renaming the various uses of tag_t::map_type, fair::graph::node_construction_param, ... definitions to that.
  • The choice of having ports that are statically and/or dynamically defined should be dependent on the base class but IMO, always use the node<T> CTRP-base class (N.B. to be renamed -> block<T> to be consistent with existing GR3 naming conventions)
  • the hier_node (I guess soon named hier_block or perhaps sub-graph ) concept is good and one of the two possible/needed implementations ... we need to look for a nice API where users can declare the sub-graph in the sense of 'grouping' and or sub-domain with its own scheduler.

@ivan-cukic what do you think?

include/node_registry.hpp Outdated Show resolved Hide resolved
test/qa_dynamic_node.cpp Show resolved Hide resolved
fg::OUT<T> _output_port;

public:
multi_adder(std::size_t input_ports_size) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be handled with node parameters and the initialisation been done in

  uint32_t n_input_ports = 3;
public:
    void init(const tag_t::map_type &old_setting, const tag_t::map_type &new_setting) noexcept { /*...*/ }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For later, added a comment - it needs dynamic ports to be supported by fg::node


~multi_adder() override = default;

std::string_view
Copy link
Member

Choose a reason for hiding this comment

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

why not use node::set_name(..)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For later, added a comment - it needs dynamic ports to be supported by fg::node

}

virtual fg::work_return_t
work() override {
Copy link
Member

Choose a reason for hiding this comment

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

if you could add a 'TODO' comment, we should aim to merge this logic with the existing node::work() implementation.

@wirew0rm also setup an issue for this: #81

Copy link
Member

Choose a reason for hiding this comment

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

... notably because this does not include tag handling and we should expose only the 'work()' function to the users on rare and very special cases.

}

virtual void *
raw() override {
Copy link
Member

Choose a reason for hiding this comment

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

is this needed? the node_model has this definition as well...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

raw exists in order for graph to be able to find nodes (a node doesn't know it is wrapped in something, when it wants to communicate with graph, the graph needs to be able to 'see through' the wrappers.

namespace fg = fair::graph;

template<typename T>
class multi_adder : public fg::node_model {
Copy link
Member

Choose a reason for hiding this comment

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

derive it from node?

Copy link
Member

Choose a reason for hiding this comment

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

node-model can still wrap the strict type.

test/qa_dynamic_node.cpp Show resolved Hide resolved
test/qa_hier_node.cpp Show resolved Hide resolved
test/qa_hier_node.cpp Show resolved Hide resolved
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 16:34 — with GitHub Actions Inactive
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.

LGTM 👍 !

The things that need to be tackled in later PR are documented.

@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 17:48 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 17:49 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 17:49 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 17:49 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 17:49 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 17:49 — with GitHub Actions Inactive
pmtv doesn't like std::size_t on web assembly
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic temporarily deployed to configure coverage June 16, 2023 18:05 — with GitHub Actions Inactive
@ivan-cukic ivan-cukic merged commit a1d45da into main Jun 16, 2023
@ivan-cukic ivan-cukic deleted the ivan/plugin-improvements branch June 16, 2023 18:20
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.

[5pt,9pt] GR Flowgraph: Block registry, runtime data, bootstraping the flow-graph from static reflection data
2 participants