Refactor GP node handling

Make sure that UI Blocks don't hold references to GR nodes after creation (i.e. once scheduler started). Exceptions are the drawable nodes (toolbar/plots), which are currently both part of the graph and also accessed from the UI.
fair-acc · Apr 18, 2024 · ad725ac · ad725ac
1 parent 7df1b3d
commit ad725ac
Show file tree

Hide file tree

Showing 24 changed files with 324 additions and 368 deletions.
diff --git a/cmake/Dependencies.cmake b/cmake/Dependencies.cmake
@@ -9,7 +9,7 @@ FetchContent_Declare(
 FetchContent_Declare(
         graph-prototype
         GIT_REPOSITORY https://github.com/fair-acc/graph-prototype.git
-        GIT_TAG 3cb5c2aff7046bc3760ccd8f8b701cbde361ecfd # main as of 2024-04-05
+        GIT_TAG frank/wip/od-refactoring
 )
 
 FetchContent_Declare(

diff --git a/src/service/dashboard/defaultDashboard.flowgraph b/src/service/dashboard/defaultDashboard.flowgraph
@@ -1,33 +1,29 @@
 blocks:
   - name: FFT
-    id: FFT
+    id: gr::blocks::fft::FFT
   - name: sum sigs
     id: opendigitizer::Arithmetic
   - name: sine source 1
     id: opendigitizer::SineSource
     parameters:
       frequency: 0.100000
-  - name: source for sink 1
-    id: sink_source
-  - name: source for sink 2
-    id: sink_source
   - name: remote source 1
     id: opendigitizer::RemoteSource
     parameters:
       remote_uri: https://localhost:8080/GnuRadio/Acquisition?channelNameFilter=test
   - name: sink 1
-    id: sink
+    id: opendigitizer::DataSink
   - name: sink 2
-    id: sink
+    id: opendigitizer::DataSink
   - name: sink 3
-    id: sink
+    id: opendigitizer::DataSink
   - name: sink 4
-    id: sink
+    id: opendigitizer::DataSink
 connections:
   - [sine source 1, 0, FFT, 0]
   - [FFT, 0, sink 1, 0]
   - [sine source 1, 0, sum sigs, 0]
-  - [source for sink 1, 0, sum sigs, 1]
+  - [FFT, 0, sum sigs, 1]
   - [sum sigs, 0, sink 2, 0]
   - [sine source 1, 0, sink 3, 0]
   - [remote source 1, 0, sink 4, 0]
diff --git a/src/ui/CMakeLists.txt b/src/ui/CMakeLists.txt
@@ -183,6 +183,7 @@ target_link_libraries(
           gnuradio-algorithm
           gr-basic
           gr-fourier
+          gr-testing
           fftw
           vir
           pmtv)

diff --git a/src/ui/app.hpp b/src/ui/app.hpp
@@ -160,7 +160,9 @@ class App {
         std::unique_ptr<Handler> handler;
     };
 
-    SchedWrapper _scheduler;
+    SchedWrapper                  _scheduler;
+    std::vector<gr::BlockModel *> _toolbarBlocks;
+    std::vector<gr::BlockModel *> _plotBlocks;
 
 public:
     App() noexcept { setStyle(Style::Light); }

diff --git a/src/ui/app_header/CMakeLists.txt b/src/ui/app_header/CMakeLists.txt
@@ -1,7 +1,7 @@
 add_library(
-    app_header INTERFACE
-    "fair_header.hpp"
-    PopupMenu.hpp
-)
+  app_header
+  INTERFACE
+  "fair_header.hpp"
+  PopupMenu.hpp)
 target_include_directories(app_header INTERFACE ${CMAKE_CURRENT_SOURCE_DIR})
 target_link_libraries(app_header INTERFACE stb fmt)
diff --git a/src/ui/assets/sampleDashboards/DemoDashboard.grc b/src/ui/assets/sampleDashboards/DemoDashboard.grc
@@ -2,7 +2,7 @@ blocks:
   - name: sum sigs
     id: opendigitizer::Arithmetic
   - name: FFT
-    id: FFT
+    id: gr::blocks::fft::FFT
   - name: sine source 1
     id: opendigitizer::SineSource
     parameters:
@@ -12,21 +12,13 @@ blocks:
     parameters:
       frequency: 1.300000
   - name: sink 1
-    id: sink
-  - name: source for sink 1
-    id: sink_source
+    id: opendigitizer::DataSink
   - name: sink 2
-    id: sink
-  - name: source for sink 2
-    id: sink_source
+    id: opendigitizer::DataSink
   - name: sink 3
-    id: sink
-  - name: source for sink 3
-    id: sink_source
+    id: opendigitizer::DataSink
   - name: sink 4
-    id: sink
-  - name: source for sink 4
-    id: sink_source
+    id: opendigitizer::DataSink
 connections:
   - [sine source 1, 0, sum sigs, 0]
   - [sine source 3, 0, sum sigs, 1]

diff --git a/src/ui/assets/sampleDashboards/ExtendedDemoDashboard.grc b/src/ui/assets/sampleDashboards/ExtendedDemoDashboard.grc
@@ -6,7 +6,7 @@ blocks:
   - name: sum sigs3
     id: opendigitizer::Arithmetic
   - name: FFT
-    id: FFT
+    id: gr::blocks::fft::FFT
   - name: sine source 3
     id: opendigitizer::SineSource
     parameters:
@@ -23,22 +23,18 @@ blocks:
     id: opendigitizer::SineSource
     parameters:
       frequency: 0.400000
-  - name: source for sink 1
-    id: sink_source
-  - name: source for sink 2
-    id: sink_source
   - name: sink 1
-    id: sink
+    id: opendigitizer::DataSink
   - name: sink 2
-    id: sink
+    id: opendigitizer::DataSink
   - name: sink 3
-    id: sink
+    id: opendigitizer::DataSink
   - name: sink 4
-    id: sink
+    id: opendigitizer::DataSink
   - name: sink 5
-    id: sink
+    id: opendigitizer::DataSink
   - name: sink 6
-    id: sink
+    id: opendigitizer::DataSink
 connections:
   - [sine source 3, 0, sum sigs1, 0]
   - [sine source 4, 0, sum sigs1, 1]

diff --git a/src/ui/blocks/Arithmetic.hpp b/src/ui/blocks/Arithmetic.hpp
@@ -38,4 +38,6 @@ struct Arithmetic : public gr::Block<Arithmetic<T>> {
 
 ENABLE_REFLECTION_FOR_TEMPLATE(opendigitizer::Arithmetic, in1, in2, out, operation)
 
+auto registerArithmetic = gr::registerBlock<opendigitizer::Arithmetic, float, double>(gr::globalBlockRegistry());
+
 #endif
diff --git a/src/ui/blocks/RemoteSource.hpp b/src/ui/blocks/RemoteSource.hpp
@@ -16,9 +16,9 @@
 namespace opendigitizer {
 
 template<typename T>
-    requires std::is_same_v<T, float>
+    requires std::is_floating_point_v<T>
 struct RemoteSource : public gr::Block<RemoteSource<T>> {
-    gr::PortOut<float>          out;
+    gr::PortOut<T>              out;
     std::string                 remote_uri;
     std::string                 signal_name;
     std::string                 signal_unit;
@@ -53,7 +53,11 @@ struct RemoteSource : public gr::Block<RemoteSource<T>> {
             auto in = std::span<const float>(d.acq.channelValue.begin(), d.acq.channelValue.end());
             in      = in.subspan(d.read, std::min(output.size() - written, in.size() - d.read));
 
-            std::copy(in.begin(), in.end(), output.begin() + written);
+            if constexpr (std::is_same_v<T, float>) {
+                std::ranges::copy(in, output.begin() + written);
+            } else {
+                std::ranges::transform(in, output.begin() + written, [](float v) { return static_cast<T>(v); });
+            }
             written += in.size();
             d.read += in.size();
             if (d.read == d.acq.channelValue.size()) {
@@ -121,4 +125,6 @@ struct RemoteSource : public gr::Block<RemoteSource<T>> {
 
 ENABLE_REFLECTION_FOR_TEMPLATE(opendigitizer::RemoteSource, out, remote_uri, signal_name, signal_unit, signal_min, signal_max)
 
+auto registerRemoteSource = gr::registerBlock<opendigitizer::RemoteSource, float, double>(gr::globalBlockRegistry());
+
 #endif
diff --git a/src/ui/blocks/SineSource.hpp b/src/ui/blocks/SineSource.hpp
@@ -73,4 +73,6 @@ struct SineSource : public gr::Block<SineSource<T>, gr::BlockingIO<true>> {
 
 ENABLE_REFLECTION_FOR_TEMPLATE(opendigitizer::SineSource, out, freqIn, freqOut, frequency)
 
+auto registerSineSource = gr::registerBlock<opendigitizer::SineSource, float, double>(gr::globalBlockRegistry());
+
 #endif
diff --git a/src/ui/cmake/Dependencies.cmake b/src/ui/cmake/Dependencies.cmake
@@ -21,12 +21,6 @@ FetchContent_Declare(
     GIT_TAG         v0.9.3 # latest as of 2023-12-19
 )
 
-FetchContent_Declare(
-    yaml-cpp
-    GIT_REPOSITORY  https://github.com/jbeder/yaml-cpp.git
-    GIT_TAG         yaml-cpp-0.7.0
-)
-
 FetchContent_Declare(
     plf_colony
     GIT_REPOSITORY  https://github.com/mattreecebentley/plf_colony.git
@@ -49,10 +43,10 @@ FetchContent_Declare(
 FetchContent_Declare(
     graph-prototype
     GIT_REPOSITORY https://github.com/fair-acc/graph-prototype.git
-    GIT_TAG 3cb5c2aff7046bc3760ccd8f8b701cbde361ecfd # main as of 2024-04-05
+    GIT_TAG frank/wip/od-refactoring
 )
 
-FetchContent_MakeAvailable(imgui implot imgui-node-editor yaml-cpp stb opencmw-cpp plf_colony graph-prototype)
+FetchContent_MakeAvailable(imgui implot imgui-node-editor stb opencmw-cpp plf_colony graph-prototype)
 
 if (NOT EMSCRIPTEN)
     find_package(SDL2 REQUIRED)

diff --git a/src/ui/dashboard.cpp b/src/ui/dashboard.cpp
@@ -253,16 +253,6 @@ DataSink *Dashboard::createSink() {
     return sinkptr;
 }
 
-DataSinkSource *Dashboard::createSource() {
-    int  n         = localFlowGraph.sourceBlocks().size() + 1;
-    auto name      = fmt::format("source {}", n);
-    auto source    = std::make_unique<DigitizerUi::DataSinkSource>(name);
-    auto sourceptr = source.get();
-    name           = fmt::format("source for sink {}", n);
-    localFlowGraph.addSourceBlock(std::move(source));
-    return sourceptr;
-}
-
 void Dashboard::setNewDescription(const std::shared_ptr<DashboardDescription> &desc) {
     m_desc = desc;
 }
@@ -272,12 +262,19 @@ void Dashboard::load() {
         fetch(
                 m_desc->source, m_desc->filename, { What::Flowgraph, What::Dashboard },
                 [_this = shared()](std::array<std::string, 2> &&data) {
-                    _this->localFlowGraph.parse(std::move(data[0]));
-                    // Load is called after parsing the flowgraph so that we already have the list of sources
-                    _this->doLoad(data[1]);
+                    try {
+                        _this->localFlowGraph.parse(std::move(data[0]));
+                        // Load is called after parsing the flowgraph so that we already have the list of sources
+                        _this->doLoad(data[1]);
+                    } catch (const std::exception &e) {
+                        // TODO show error message
+                        fmt::print(std::cerr, "Error: {}", e.what());
+                        App::instance().closeDashboard();
+                    }
                 },
                 [_this = shared()]() {
-                    fmt::print("Invalid flowgraph for dashboard {}/{}\n", _this->m_desc->source->path, _this->m_desc->filename);
+                    // TODO show error message
+                    fmt::print(std::cerr, "Invalid flowgraph for dashboard {}/{}\n", _this->m_desc->source->path, _this->m_desc->filename);
                     App::instance().closeDashboard();
                 });
     } else {
@@ -286,35 +283,21 @@ void Dashboard::load() {
 }
 
 void Dashboard::doLoad(const std::string &desc) {
-    YAML::Node tree = YAML::Load(desc);
-
-    auto       path = std::filesystem::path(m_desc->source->path) / m_desc->filename;
+    YAML::Node tree    = YAML::Load(desc);
 
-#ifdef NDEBUG
-#define ERROR_RETURN(msg) \
-    { \
-        fmt::print("Error parsing YAML {}: {}\n{}\n", path.native(), msg, desc); \
-        abort(); \
-    }
-#else
-#define ERROR_RETURN(msg) \
-    { \
-        fmt::print("Error parsing YAML {}: {}\n{}\n", path.native(), msg, desc); \
-        return; \
-    }
-#endif
+    auto       path    = std::filesystem::path(m_desc->source->path) / m_desc->filename;
 
-    auto sources = tree["sources"];
-    if (!sources || !sources.IsSequence()) ERROR_RETURN("sources entry invalid");
+    auto       sources = tree["sources"];
+    if (!sources || !sources.IsSequence()) throw std::runtime_error("sources entry invalid");
 
     for (const auto &s : sources) {
-        if (!s.IsMap()) ERROR_RETURN("source is no map");
+        if (!s.IsMap()) throw std::runtime_error("source is no map");
 
         auto block = s["block"];
         auto port  = s["port"];
         auto name  = s["name"];
         auto color = s["color"];
-        if (!block || !block.IsScalar() || !port || !port.IsScalar() || !name || !name.IsScalar() || !color || !color.IsScalar()) ERROR_RETURN("invalid source color definition");
+        if (!block || !block.IsScalar() || !port || !port.IsScalar() || !name || !name.IsScalar() || !color || !color.IsScalar()) std::runtime_error("invalid source color definition");
 
         auto blockStr = block.as<std::string>();
         auto portNum  = port.as<int>();
@@ -334,29 +317,29 @@ void Dashboard::doLoad(const std::string &desc) {
     }
 
     auto plots = tree["plots"];
-    if (!plots || !plots.IsSequence()) ERROR_RETURN("plots invalid");
+    if (!plots || !plots.IsSequence()) throw std::runtime_error("plots invalid");
 
     for (const auto &p : plots) {
-        if (!p.IsMap()) ERROR_RETURN("plots is not map");
+        if (!p.IsMap()) std::runtime_error("plots is not map");
 
         auto name        = p["name"];
         auto axes        = p["axes"];
         auto plotSources = p["sources"];
         auto rect        = p["rect"];
-        if (!name || !name.IsScalar() || !axes || !axes.IsSequence() || !plotSources || !plotSources.IsSequence() || !rect || !rect.IsSequence() || rect.size() != 4) ERROR_RETURN("invalid plot definition");
+        if (!name || !name.IsScalar() || !axes || !axes.IsSequence() || !plotSources || !plotSources.IsSequence() || !rect || !rect.IsSequence() || rect.size() != 4) throw std::runtime_error("invalid plot definition");
 
         m_plots.emplace_back();
         auto &plot = m_plots.back();
         plot.name  = name.as<std::string>();
 
         for (const auto &a : axes) {
-            if (!a.IsMap()) ERROR_RETURN("axes is no map");
+            if (!a.IsMap()) throw std::runtime_error("axes is no map");
 
             auto axis = a["axis"];
             auto min  = a["min"];
             auto max  = a["max"];
 
-            if (!axis || !axis.IsScalar() || !min || !min.IsScalar() || !max || !max.IsScalar()) ERROR_RETURN("invalid axis definition");
+            if (!axis || !axis.IsScalar() || !min || !min.IsScalar() || !max || !max.IsScalar()) throw std::runtime_error("invalid axis definition");
 
             plot.axes.push_back({});
             auto &ax      = plot.axes.back();
@@ -377,7 +360,7 @@ void Dashboard::doLoad(const std::string &desc) {
         }
 
         for (const auto &s : plotSources) {
-            if (!s.IsScalar()) ERROR_RETURN("plot source is no scalar");
+            if (!s.IsScalar()) throw std::runtime_error("plot source is no scalar");
 
             auto str = s.as<std::string>();
             plot.sourceNames.push_back(str);
@@ -393,8 +376,6 @@ void Dashboard::doLoad(const std::string &desc) {
     App::instance().fgItem.setSettings(&localFlowGraph, fgLayout && fgLayout.IsScalar() ? fgLayout.as<std::string>() : std::string{});
 
     loadPlotSources();
-
-#undef ERROR_RETURN
 }
 
 void Dashboard::save() {

diff --git a/src/ui/dashboard.hpp b/src/ui/dashboard.hpp
@@ -26,8 +26,6 @@ class Block;
 class FlowGraph;
 struct DashboardDescription;
 class DataSink;
-class DataSinkSource;
-
 struct DashboardSource {
     ~DashboardSource() noexcept;
 
@@ -132,17 +130,16 @@ class Dashboard : public std::enable_shared_from_this<Dashboard> {
         void                        reload();
         void                        execute();
     };
-    void            addRemoteService(std::string_view uri);
-    void            saveRemoteServiceFlowgraph(Service *s);
+    void         addRemoteService(std::string_view uri);
+    void         saveRemoteServiceFlowgraph(Service *s);
 
-    inline auto    &remoteServices() { return m_services; }
+    inline auto &remoteServices() { return m_services; }
 
-    DataSink       *createSink();
-    DataSinkSource *createSource();
+    DataSink    *createSink();
 
-    void            loadPlotSources();
+    void         loadPlotSources();
 
-    FlowGraph       localFlowGraph;
+    FlowGraph    localFlowGraph;
 
 private:
     void                                  doLoad(const std::string &desc);
Original file line number	Diff line number	Diff line change
Expand Up		@@ -38,4 +38,6 @@ struct Arithmetic : public gr::Block<Arithmetic<T>> {

		ENABLE_REFLECTION_FOR_TEMPLATE(opendigitizer::Arithmetic, in1, in2, out, operation)

		auto registerArithmetic = gr::registerBlock<opendigitizer::Arithmetic, float, double>(gr::globalBlockRegistry());

		#endif
Original file line number	Diff line number	Diff line change
Expand Up		@@ -73,4 +73,6 @@ struct SineSource : public gr::Block<SineSource<T>, gr::BlockingIO<true>> {

		ENABLE_REFLECTION_FOR_TEMPLATE(opendigitizer::SineSource, out, freqIn, freqOut, frequency)

		auto registerSineSource = gr::registerBlock<opendigitizer::SineSource, float, double>(gr::globalBlockRegistry());

		#endif