From 6a391c5de4ddb9b8959d89e49bc90759d58a0620 Mon Sep 17 00:00:00 2001
From: Tomasz Kozak <tomasz.kozak@desy.de>
Date: Sat, 1 Oct 2022 12:23:30 +0200
Subject: [PATCH] Rework StatusAggregator

---
 Modules/include/StatusAggregator.h            |   2 +-
 Modules/include/StatusWithMessage.h           |  18 +-
 Modules/src/StatusAggregator.cc               | 159 ++++++------------
 tests/executables_src/testStatusAggregator.cc |  36 ++--
 4 files changed, 85 insertions(+), 130 deletions(-)

diff --git a/Modules/include/StatusAggregator.h b/Modules/include/StatusAggregator.h
index 82d6adfd..def119fd 100644
--- a/Modules/include/StatusAggregator.h
+++ b/Modules/include/StatusAggregator.h
@@ -87,7 +87,7 @@ namespace ChimeraTK {
     void populateStatusInput();
 
     /// Helper for populateStatusInput
-    void scanAndPopulateFromHierarchyLevel(EntityOwner& module, const std::string& namePrefix);
+    // void scanAndPopulateFromHierarchyLevel(EntityOwner& module, const std::string& namePrefix);
 
     /** Reserved tag which is used to mark aggregated status outputs (need to stop searching further down the
      *  hierarchy) */
diff --git a/Modules/include/StatusWithMessage.h b/Modules/include/StatusWithMessage.h
index b4fad07b..49b6b0d3 100644
--- a/Modules/include/StatusWithMessage.h
+++ b/Modules/include/StatusWithMessage.h
@@ -4,6 +4,7 @@
 
 #include "ScalarAccessor.h"
 #include "StatusAccessor.h"
+#include "Utilities.h"
 #include "VariableGroup.h"
 
 #include <ChimeraTK/ControlSystemAdapter/StatusWithMessageReader.h>
@@ -51,17 +52,10 @@ namespace ChimeraTK {
     ScalarPushInput<std::string> _message; // left uninitialized, if no message source provided
 
     /// Construct StatusWithMessageInput which reads only status, not message
-    StatusWithMessageInput(ApplicationModule* owner, std::string name, const std::string& description,
+    StatusWithMessageInput(ApplicationModule* owner, const std::string& qualifiedName, const std::string& description,
         const std::unordered_set<std::string>& tags = {})
-    : VariableGroup(owner, name, "", tags), _status(this, name, description) {
-      hasMessageSource = false;
-      _statusNameLong = description;
-    }
-
-    [[deprecated]] StatusWithMessageInput(ApplicationModule* owner, std::string name, const std::string& description,
-        HierarchyModifier hierarchyModifier = HierarchyModifier::none, const std::unordered_set<std::string>& tags = {})
-    : VariableGroup(owner, name, "", tags), _status(this, name, description) {
-      applyHierarchyModifierToName(hierarchyModifier);
+    : VariableGroup(owner, Utilities::getPathName(qualifiedName), "", tags),
+      _status(this, Utilities::getUnqualifiedName(qualifiedName), description) {
       hasMessageSource = false;
       _statusNameLong = description;
     }
@@ -70,7 +64,9 @@ namespace ChimeraTK {
     ///  If not given, it is selected automatically by the naming convention
     void setMessageSource(std::string msgInputName = "") {
       // at the time this function is called, TransferElement impl is not yet set, so don't look there for name
-      if(msgInputName == "") msgInputName = ((VariableNetworkNode)_status).getName() + "_message";
+      if(msgInputName.empty()) {
+        msgInputName = ((VariableNetworkNode)_status).getName() + "_message";
+      }
       // late initialization of _message
       _message = ScalarPushInput<std::string>(this, msgInputName, "", "");
       hasMessageSource = true;
diff --git a/Modules/src/StatusAggregator.cc b/Modules/src/StatusAggregator.cc
index 8b3395a6..021eb130 100644
--- a/Modules/src/StatusAggregator.cc
+++ b/Modules/src/StatusAggregator.cc
@@ -15,13 +15,11 @@ namespace ChimeraTK {
   : ApplicationModule(owner, ".", description, outputTags), _output(this, name), _mode(mode),
     _tagsToAggregate(tagsToAggregate) {
     // check maximum size of tagsToAggregate
-    if(tagsToAggregate.size() > 1) {
+    if(_tagsToAggregate.size() > 1) {
       throw ChimeraTK::logic_error("StatusAggregator: List of tagsToAggregate must contain at most one tag.");
     }
-
     // add reserved tag tagAggregatedStatus to the status output, so it can be detected by other StatusAggregators
     _output._status.addTag(tagAggregatedStatus);
-
     // search the variable tree for StatusOutputs and create the matching inputs
     populateStatusInput();
   }
@@ -29,120 +27,75 @@ namespace ChimeraTK {
   /********************************************************************************************************************/
 
   void StatusAggregator::populateStatusInput() {
-    throw ChimeraTK::logic_error("THIS FUNCTION NEEDS TO BE REWRITTEN TO USE THE MODEL.");
-    /*    scanAndPopulateFromHierarchyLevel(*getOwner(), ".");
-
-        // Special treatment for DeviceModules, because they are formally not owned by the Application: If we are
-        // aggregating all tags at the top-level of the Application, include the status outputs of all DeviceModules.
-        if(getOwner() == &Application::getInstance() && _tagsToAggregate.empty()) {
-          for(auto& p : Application::getInstance().deviceModuleMap) {
-            scanAndPopulateFromHierarchyLevel(p.second->deviceError, ".");
+    auto model = dynamic_cast<ModuleGroup*>(_owner)->getModel();
+
+    // set of potential inputs for this StatusAggregator instance
+    std::set<std::string> inputPathsSet;
+    // set of inputs for other, already created, StatusAggregator instances
+    std::set<std::string> anotherStatusAgregatorInputSet;
+    // map which assigns fully qualified path of StatusAggregator output to the ully qualified path StatusAggregator output message
+    std::map<std::string, std::string> statusToMessagePathsMap;
+
+    auto scanModel = [&](auto proxy) {
+      if constexpr(ChimeraTK::Model::isApplicationModule(proxy)) {
+        StatusAggregator* staAggPtr = dynamic_cast<StatusAggregator*>(&proxy.getApplicationModule());
+        if(staAggPtr != nullptr) {
+          if(staAggPtr == this) return;
+
+          if(_tagsToAggregate == staAggPtr->_tagsToAggregate) {
+            inputPathsSet.insert(staAggPtr->_output._status.getModel().getFullyQualifiedPath());
+
+            statusToMessagePathsMap[staAggPtr->_output._status.getModel().getFullyQualifiedPath()] =
+                staAggPtr->_output._message.getModel().getFullyQualifiedPath();
+
+            for(auto& anotherStatusAgregatorInput : staAggPtr->_inputs) {
+              anotherStatusAgregatorInputSet.insert(
+                  anotherStatusAgregatorInput._status.getModel().getFullyQualifiedPath());
+            }
           }
         }
-
-        // Check if no inputs are present (nothing found to aggregate)
-        if(_inputs.empty()) {
-          throw ChimeraTK::logic_error("StatusAggregator " + VariableNetworkNode(_output._status).getQualifiedName() +
-              " has not found anything to aggregate.");
-        }*/
-  }
-
-  /********************************************************************************************************************/
-
-  void StatusAggregator::scanAndPopulateFromHierarchyLevel(EntityOwner& module, const std::string& namePrefix) {
-    // a map used just for optimization of node lookup by name, which happens in inner loop below
-    std::map<std::string, VariableNetworkNode> nodesByName;
-    for(auto& node : module.getAccessorList()) {
-      nodesByName[node.getName()] = node;
-    }
-    // Search for StatusOutputs to aggregate
-    for(auto& node : module.getAccessorList()) {
-      // Filter required tags
-      // Note: findTag() cannot be used instead, since the search must be done on the original C++ hierarchy. Otherwise
-      // it is not possible to identify which StatusOutputs are aggregated by other StatusAggregators.
-      const auto& tags = node.getTags();
-      if(tags.find(StatusOutput::tagStatusOutput) == tags.end()) {
-        // StatusOutput's reserved tag not present: not a StatusOutput
-        continue;
-      }
-      bool skip = false;
-      for(const auto& tag : _tagsToAggregate) {
-        // Each tag attached to this StatusAggregator must be present at all StatusOutputs to be aggregated
-        if(tags.find(tag) == tags.end()) {
-          skip = true;
-          break;
-        }
       }
-      if(skip) continue;
 
-      // All variables with the StatusOutput::tagStatusOutput need to be feeding, otherwise someone has used the tag
-      // wrongly
-      if(node.getDirection().dir != VariableDirection::feeding) {
-        throw ChimeraTK::logic_error("BUG DETECTED: StatusOutput's reserved tag has been found on a consumer.");
-      }
-
-      // Create matching input for the found StatusOutput of the other StatusAggregator
-      // Unfortunately, the qualified name of the newly-created node is useless,
-      // so we save the original one as description for indication in a message
-      _inputs.emplace_back(this, node.getName(), node.getQualifiedName(), HierarchyModifier::hideThis,
-          std::unordered_set<std::string>{tagInternalVars});
-      // node >> _inputs.back()._status;
-      //  look for matching status message output node
-      auto result = nodesByName.find(node.getName() + "_message");
-      if(result != nodesByName.end()) {
-        // tell the StatusWithMessageInput that it should consider the message source, and connect it
-        auto statusMsgNode = result->second;
-        _inputs.back().setMessageSource();
-        // statusMsgNode >> _inputs.back()._message;
-      }
-    }
-
-    // Search for StatusAggregators among submodules
-    const auto& ml = module.getSubmoduleList();
-    for(const auto& submodule : ml) {
-      // do nothing, if it is *this
-      if(submodule == this) continue;
-
-      auto* aggregator = dynamic_cast<StatusAggregator*>(submodule);
-      if(aggregator != nullptr) {
-        // never aggregate on the same level
-        if(aggregator->getOwner() == getOwner()) {
-          continue;
+      if constexpr(ChimeraTK::Model::isVariable(proxy)) {
+        // check whether its not output of this (current) StatusAggregator. 'Current' StatusAggregator output is also
+        // visible in the scanned model and should be ignored
+        if(proxy.getFullyQualifiedPath().compare(_output._status.getModel().getFullyQualifiedPath()) == 0) {
+          return;
         }
 
-        // if another StatusAggregator has been found, check tags
-        bool skip = false;
-        const auto& tags = aggregator->_tagsToAggregate;
-        for(const auto& tag : _tagsToAggregate) {
-          // Each tag attached to this StatusAggregator must be present at all StatusOutputs to be aggregated
-          if(tags.find(tag) == tags.end()) {
-            skip = true;
-            break;
+        auto tags = proxy.getTags();
+        // find another aggregator output - this is already covered by checking if given module is a StatusAggregator
+        if(tags.find(StatusAggregator::tagAggregatedStatus) != tags.end()) {
+          return;
+        }
+        // find status output - this is potential candidate to be aggregated
+        if(tags.find(StatusOutput::tagStatusOutput) != tags.end()) {
+          for(const auto& tagToAgregate : _tagsToAggregate) {
+            // Each tag attached to this StatusAggregator must be present at all StatusOutputs to be aggregated
+            if(tags.find(tagToAgregate) == tags.end()) {
+              return;
+            }
           }
+          inputPathsSet.insert(proxy.getFullyQualifiedPath());
         }
-        if(skip) continue;
-
-        // aggregate the StatusAggregator's result
-        auto statusNode = VariableNetworkNode(aggregator->_output._status);
-        _inputs.emplace_back(this, statusNode.getName(), "", HierarchyModifier::hideThis,
-            std::unordered_set<std::string>{tagInternalVars});
-        // statusNode >> _inputs.back()._status;
-        auto msgNode = VariableNetworkNode(aggregator->_output._message);
-        _inputs.back().setMessageSource();
-        // msgNode >> _inputs.back()._message;
-        return;
       }
+    };
+
+    model.visit(scanModel, ChimeraTK::Model::keepApplicationModules || ChimeraTK::Model::keepProcessVariables,
+        ChimeraTK::Model::breadthFirstSearch, ChimeraTK::Model::keepOwnership);
+
+    for(auto& pathToBeRemoved : anotherStatusAgregatorInputSet) {
+      inputPathsSet.erase(pathToBeRemoved);
     }
 
-    // If no (matching) StatusAggregator is found, recurse into sub-modules
-    for(const auto& submodule : ml) {
-      if(dynamic_cast<StatusAggregator*>(submodule) != nullptr) continue; // StatusAggregators are already handled
-      scanAndPopulateFromHierarchyLevel(*submodule, namePrefix + "/" + submodule->getName());
+    for(auto& pathToBeAggregated : inputPathsSet) {
+      _inputs.emplace_back(
+          this, pathToBeAggregated, pathToBeAggregated, std::unordered_set<std::string>{tagInternalVars});
+      if(!statusToMessagePathsMap[pathToBeAggregated].empty())
+        _inputs.back().setMessageSource(statusToMessagePathsMap[pathToBeAggregated]);
     }
   }
 
-  /********************************************************************************************************************/
-
   int StatusAggregator::getPriority(StatusOutput::Status status) const {
     using Status = StatusOutput::Status;
 
diff --git a/tests/executables_src/testStatusAggregator.cc b/tests/executables_src/testStatusAggregator.cc
index fc138b69..213f7dae 100644
--- a/tests/executables_src/testStatusAggregator.cc
+++ b/tests/executables_src/testStatusAggregator.cc
@@ -19,6 +19,12 @@ namespace ctk = ChimeraTK;
 
 struct StatusGenerator : ctk::ApplicationModule {
   using ctk::ApplicationModule::ApplicationModule;
+
+  StatusGenerator(ctk::ModuleGroup* owner, const std::string& name, const std::string& description,
+      const std::unordered_set<std::string>& tags = {})
+  : ApplicationModule(owner, name, description, tags) {
+    // std::cout << "The name: " << getName() << std::endl;
+  }
   ctk::StatusOutput status{this, getName(), ""};
   void mainLoop() override {}
 };
@@ -29,17 +35,17 @@ struct TestApplication : ctk::Application {
   TestApplication() : Application("testApp") {}
   ~TestApplication() override { shutdown(); }
 
-  StatusGenerator s{this, ".", "Status"};
+  StatusGenerator s{this, "s", "Status"};
 
   struct OuterGroup : ctk::ModuleGroup {
     using ctk::ModuleGroup::ModuleGroup;
 
-    StatusGenerator s1{this, ".", "Status 1"};
-    StatusGenerator s2{this, ".", "Status 2"};
+    StatusGenerator s1{this, "s1", "Status 1"};
+    StatusGenerator s2{this, "s2", "Status 2"};
 
     struct InnerGroup : ctk::ModuleGroup {
       using ctk::ModuleGroup::ModuleGroup;
-      StatusGenerator s{this, ".", "Status"};
+      StatusGenerator s{this, "s", "Status"};
       StatusGenerator deep{this, "deep", "Status"};
     };
     InnerGroup innerGroup1{this, "InnerGroup1", ""};
@@ -51,7 +57,7 @@ struct TestApplication : ctk::Application {
       this, "Aggregated/status", "aggregated status description", ctk::StatusAggregator::PriorityMode::fwko};
 };
 
-/**********************************************************************************************************************/
+///**********************************************************************************************************************/
 
 BOOST_AUTO_TEST_CASE(testSingleNoTags) {
   std::cout << "testSingleNoTags" << std::endl;
@@ -107,8 +113,8 @@ struct TestPrioApplication : ctk::Application {
   TestPrioApplication() : Application("testApp") {}
   ~TestPrioApplication() override { shutdown(); }
 
-  StatusGenerator s1{this, ".", "Status 1"};
-  StatusGenerator s2{this, ".", "Status 2"};
+  StatusGenerator s1{this, "sg1/internal", "Status 1"};
+  StatusGenerator s2{this, "sg2/external", "Status 2"};
 
   ctk::StatusAggregator aggregator;
 };
@@ -213,13 +219,13 @@ struct TestApplication2Levels : ctk::Application {
   TestApplication2Levels() : Application("testApp") {}
   ~TestApplication2Levels() override { shutdown(); }
 
-  StatusGenerator s{this, ".", "Status"};
+  StatusGenerator s{this, "s", "Status"};
 
   struct OuterGroup : ctk::ModuleGroup {
     using ctk::ModuleGroup::ModuleGroup;
 
-    StatusGenerator s1{this, ".", "Status 1"};
-    StatusGenerator s2{this, ".", "Status 2"};
+    StatusGenerator s1{this, "s1", "Status 1"};
+    StatusGenerator s2{this, "s2", "Status 2"};
 
     ctk::StatusAggregator extraAggregator{
         this, "/Aggregated/extraStatus", "aggregated status description", ctk::StatusAggregator::PriorityMode::ofwk};
@@ -230,10 +236,10 @@ struct TestApplication2Levels : ctk::Application {
       this, "Aggregated/status", "aggregated status description", ctk::StatusAggregator::PriorityMode::fwko};
 };
 
-/**********************************************************************************************************************/
+///**********************************************************************************************************************/
 
 BOOST_AUTO_TEST_CASE(testTwoLevels) {
-  std::cout << "testTwoLevels" << std::endl;
+  std::cout << "testTwoLevels" << std::endl << std::endl << std::endl;
   TestApplication2Levels app;
 
   ctk::TestFacility test(app);
@@ -293,8 +299,8 @@ struct TestApplicationTags : ctk::Application {
   struct OuterGroup : ctk::ModuleGroup {
     using ctk::ModuleGroup::ModuleGroup;
 
-    StatusGenerator sA{this, ".", "Status 1", ctk::TAGS{"A"}};
-    StatusGenerator sAB{this, ".", "Status 2", {"A", "B"}};
+    StatusGenerator sA{this, "sA", "Status 1", ctk::TAGS{"A"}};
+    StatusGenerator sAB{this, "sAB", "Status 2", {"A", "B"}};
 
     ctk::StatusAggregator aggregateA{
         this, "aggregateA", "aggregated status description", ctk::StatusAggregator::PriorityMode::fwko, {"A"}};
@@ -408,7 +414,7 @@ BOOST_AUTO_TEST_CASE(testStatusMessage) {
   innerStatus.readLatest();
   innerStatusMessage.readLatest();
   BOOST_CHECK_EQUAL(int(status), int(ctk::StatusOutput::Status::FAULT));
-  const char* faultString = "/testApp/OuterGroup/s2/s2 switched to FAULT";
+  const char* faultString = "/OuterGroup/s2/s2 switched to FAULT";
   BOOST_CHECK_EQUAL(std::string(statusMessage), faultString);
   BOOST_CHECK_EQUAL(int(innerStatus), int(ctk::StatusOutput::Status::FAULT));
   BOOST_CHECK_EQUAL(std::string(innerStatusMessage), faultString);
-- 
GitLab