Merge pull request #211 from ChimeraTK/wip-mhier-noticket-status-monitor-interface

modernise StatusMonitor interface

Modules/include/StatusMonitor.h

@@ -42,6 +42,8 @@ namespace ChimeraTK {
    *  type of the variable to be monitored. A non-template base class
    *  facilitates checking for the type in the StatusAggregator, which
    *  needs to identify any StatusMonitor.
+   *  
+   *  FIXME This distinction between StatusMonitorImpl and StatusMonitor is no longer required. Merge the classes!
    */
   struct StatusMonitor : ApplicationModule {
     StatusMonitor(EntityOwner* owner, const std::string& name, const std::string& description, const std::string& input,
@@ -217,30 +219,91 @@ namespace ChimeraTK {
     }
   };
 
-  /** Module for status monitoring of an exact value.
- * If value monitored is not exactly the same. an fault state will be
- * reported.*/
+  /** 
+   *  Module for status monitoring of an exact value.
+   *  
+   *  If monitored input value is not exactly the same as the requiredValue, a fault state will be reported. If the
+   *  parameter variable "disable" is set to a non-zero value, the monitoring is disabled and the output status is
+   *  always OFF.
+   *  
+   *  Note: It is strongly recommended to use this monitor only for integer data types or strings, as floating point
+   *  data types should never be compared with exact equality.
+   */
   template<typename T>
-  struct ExactMonitor : public StatusMonitorImpl<T> {
-    using StatusMonitorImpl<T>::StatusMonitorImpl;
-    /**FAULT state if value is not equal to requiredValue*/
-    ScalarPushInput<T> requiredValue{this, "requiredValue", "", "", StatusMonitor::_parameterTags};
+  struct ExactMonitor : ApplicationModule {
+    /** 
+     *  Constructor for exact monitoring module.
+     *  
+     *  inputPath: qualified path of the variable to monitor
+     *  outputPath: qualified path of the status output variable
+     *  parameterPath: qualified path of the VariableGroup holding the parameter variables requiredValue and disable
+     *  
+     *  All qualified paths can be either relative or absolute to the given owner. See HierarchyModifyingGroup for
+     *  more details.
+     */
+    ExactMonitor(EntityOwner* owner, const std::string& inputPath, const std::string& outputPath,
+        const std::string& parameterPath, const std::string& description,
+        const std::unordered_set<std::string>& outputTags = {},
+        const std::unordered_set<std::string>& parameterTags = {})
+    : ExactMonitor(owner, inputPath, outputPath, parameterPath + "/requiredValue", parameterPath + "/disable",
+          description, outputTags, parameterTags) {}
 
-    /**This is where state evaluation is done*/
+    /** 
+     *  Constructor for exact monitoring module.
+     *  
+     *  inputPath: qualified path of the variable to monitor
+     *  outputPath: qualified path of the status output variable
+     *  requiredValuePath: qualified path of the parameter variable requiredValue
+     *  disablePath: qualified path of the parameter variable disable
+     *  
+     *  All qualified paths can be either relative or absolute to the given owner. See HierarchyModifyingGroup for
+     *  more details.
+     */
+    ExactMonitor(EntityOwner* owner, const std::string& inputPath, const std::string& outputPath,
+        const std::string& requiredValuePath, const std::string& disablePath, const std::string& description,
+        const std::unordered_set<std::string>& outputTags = {},
+        const std::unordered_set<std::string>& parameterTags = {})
+    : ApplicationModule(owner, "hidden", description, HierarchyModifier::hideThis),
+      watch(this, inputPath, "", "Value to monitor"),
+      requiredValue(this, requiredValuePath, "", "Value to compare with", parameterTags),
+      disable(this, disablePath, "", "Disable the status monitor", parameterTags),
+      status(this, outputPath, "Resulting status", outputTags) {}
+
+    ExactMonitor() = default;
+
+    /** Variable to monitor */
+    ModifyHierarchy<ScalarPushInput<T>> watch;
+
+    /** The required value to compare with */
+    ModifyHierarchy<ScalarPushInput<T>> requiredValue;
+
+    /** Disable/enable the entire status monitor */
+    ModifyHierarchy<ScalarPushInput<int>> disable;
+
+    /** Result of the monitor */
+    ModifyHierarchy<StatusOutput> status;
+
+    /** This is where state evaluation is done */
     void mainLoop() {
-      /** If there is a change either in value monitored or in requiredValue, the status is re-evaluated*/
-      ReadAnyGroup group{StatusMonitorImpl<T>::oneUp.watch, StatusMonitorImpl<T>::disable, requiredValue};
+      // If there is a change either in value monitored or in requiredValue, the status is re-evaluated
+      ReadAnyGroup group{watch.value, disable.value, requiredValue.value};
       while(true) {
-        if(StatusMonitorImpl<T>::disable != 0) {
-          StatusMonitorImpl<T>::status = StatusOutput::Status::OFF;
+        StatusOutput::Status newStatus;
+        if(disable.value != 0) {
+          newStatus = StatusOutput::Status::OFF;
         }
-        else if(StatusMonitorImpl<T>::oneUp.watch != requiredValue) {
-          StatusMonitorImpl<T>::status = StatusOutput::Status::FAULT;
+        else if(watch.value != requiredValue.value) {
+          newStatus = StatusOutput::Status::FAULT;
         }
         else {
-          StatusMonitorImpl<T>::status = StatusOutput::Status::OK;
+          newStatus = StatusOutput::Status::OK;
+        }
+
+        // update only if status has changed, but always in case of initial value
+        if(status.value != newStatus || status.value.getVersionNumber() == VersionNumber{nullptr}) {
+          status.value = newStatus;
+          status.value.write();
         }
-        StatusMonitorImpl<T>::status.write();
         group.readAny();
       }
     }

include/EntityOwner.h

@@ -20,10 +20,15 @@ namespace ChimeraTK {
   class Module;
   class VirtualModule;
 
-  /** Base class for owners of other EntityOwners (e.g. Modules) and Accessors.
-   * @todo Rename this class to "Owner" and make it more generic. It should
-   * basically just implement the "Composite Pattern". The classes AccessorBase,
-   * Module and Owner should have a common base class called "Component".
+  /** 
+   *  Convenience type definition which can optionally be used as a shortcut for the type which defines a list of
+   *  tags.
+   */
+  using TAGS = const std::unordered_set<std::string>;
+
+  /** 
+   *  Base class for owners of other EntityOwners (e.g. Modules) and Accessors.
+   *  FIXME: Unify with Module class (not straight forward!).
    */
   class EntityOwner {
    public:

include/HierarchyModifyingGroup.h

@@ -8,6 +8,8 @@
 
 namespace ChimeraTK {
 
+  /********************************************************************************************************************/
+
   /**
    *  A HierarchyModifyingGroup is a VariableGroup which can easily appear at a different place in the hierarchy by
    *  specifying a qualified name. The qualifiedName can contain multiple hierarchy levels separated by slashes "/".
@@ -50,6 +52,27 @@ namespace ChimeraTK {
     std::vector<std::string> _splittedPath;
   };
 
+  /********************************************************************************************************************/
+
+  /**
+   *  Convenience version of the HierarchyModifyingGroup with exactly one variable inside. The constructor takes the
+   *  qualified name of the variable and splits it internally into the path name (for the HierarchyModifyingGroup) and
+   *  the unqialified variable name.
+   *  
+   *  The template argument must be one of the Scalar*Input, ScalarOutput classes resp. their Array counterparts.
+   */
+  template<typename ACCESSOR>
+  struct ModifyHierarchy : HierarchyModifyingGroup {
+    template<typename... Types>
+    ModifyHierarchy(Module* owner, const std::string& qualifiedName, Types... args)
+    : HierarchyModifyingGroup(owner, HierarchyModifyingGroup::getPathName(qualifiedName), ""),
+      value(this, HierarchyModifyingGroup::getUnqualifiedName(qualifiedName), args...) {}
+
+    ACCESSOR value;
+  };
+
+  /********************************************************************************************************************/
+
 } // namespace ChimeraTK
 
 #endif // HIERARCHYMODIFYINGGROUP_H

tests/executables_src/testStatusModule.cc

@@ -9,8 +9,8 @@
 #include "ApplicationModule.h"
 #include "ControlSystemModule.h"
 #include "ScalarAccessor.h"
-#include "TestFacility.h"
 #include "StatusMonitor.h"
+#include "TestFacility.h"
 
 using namespace boost::unit_test_framework;
 namespace ctk = ChimeraTK;
@@ -19,9 +19,9 @@ namespace ctk = ChimeraTK;
 template<typename T>
 struct TestApplication : public ctk::Application {
   TestApplication() : Application("testSuite") {}
-  ~TestApplication() { shutdown(); }
+  ~TestApplication() override { shutdown(); }
 
-  void defineConnections() {
+  void defineConnections() override {
     findTag(".*").connectTo(cs);                              // publish everything to CS
     findTag("MY_MONITOR").connectTo(cs["MyNiceMonitorCopy"]); // cable again, checking that the tag is applied correctly
     findTag("MON_PARAMS")
@@ -34,6 +34,24 @@ struct TestApplication : public ctk::Application {
       {"MON_OUTPUT"}, {"MON_PARAMS"}, {"MY_MONITOR"}};
 };
 
+/* dummy application - for new StatusMonitor interface */
+template<typename T>
+struct TestApplicationNewInterface : public ctk::Application {
+  TestApplicationNewInterface() : Application("testSuite") {}
+  ~TestApplicationNewInterface() override { shutdown(); }
+
+  void defineConnections() override {
+    findTag(".*").connectTo(cs); // publish everything to CS
+    findTag("MON_PARAMS")
+        .connectTo(cs["MonitorParameters"]); // cable the parameters in addition (checking that tags are set correctly)
+    findTag("MON_OUTPUT")
+        .connectTo(cs["MonitorOutput"]); // cable the parameters in addition (checking that tags are set correctly)
+  }
+  ctk::ControlSystemModule cs;
+  T monitor{this, "/input/path", "/output/path", "/parameters", "Now this is a nice monitor...",
+      ctk::TAGS{"MON_OUTPUT"}, ctk::TAGS{"MON_PARAMS"}};
+};
+
 /*********************************************************************************************************************/
 
 BOOST_AUTO_TEST_CASE(testMaxMonitor) {
@@ -552,34 +570,34 @@ BOOST_AUTO_TEST_CASE(testRangeMonitor) {
 
 BOOST_AUTO_TEST_CASE(testExactMonitor) {
   std::cout << "testExactMonitor" << std::endl;
-  TestApplication<ctk::ExactMonitor<float>> app;
+  TestApplicationNewInterface<ctk::ExactMonitor<int64_t>> app;
 
   // check that the reserved StatusOutput tag is present at the output, required for StatusAggregator integration
-  auto tags = ctk::VariableNetworkNode(app.monitor.status).getTags();
+  auto tags = ctk::VariableNetworkNode(app.monitor.status.value).getTags();
   BOOST_CHECK(tags.find(ctk::StatusOutput::tagStatusOutput) != tags.end());
 
   ctk::TestFacility test;
   test.runApplication();
   //app.dumpConnections();
 
-  auto requiredValue = test.getScalar<float>(std::string("/Monitor/requiredValue"));
-  requiredValue = 40.9;
+  auto requiredValue = test.getScalar<int64_t>(std::string("/parameters/requiredValue"));
+  requiredValue = 409;
   requiredValue.write();
   test.stepApplication();
 
-  auto watch = test.getScalar<float>(std::string("/watch"));
-  watch = 40.9;
+  auto watch = test.getScalar<int64_t>(std::string("/input/path"));
+  watch = 409;
   watch.write();
   test.stepApplication();
 
-  auto status = test.getScalar<int32_t>(std::string("/Monitor/status"));
+  auto status = test.getScalar<int32_t>(std::string("/output/path"));
   status.readLatest();
 
   //should be in OK state.
   BOOST_CHECK_EQUAL(status, static_cast<int>(ChimeraTK::StatusOutput::Status::OK));
 
   // drop in a disable test.
-  auto disable = test.getScalar<int>("/Monitor/disable");
+  auto disable = test.getScalar<int>("/parameters/disable");
   disable = 1;
   disable.write();
   test.stepApplication();
@@ -593,7 +611,7 @@ BOOST_AUTO_TEST_CASE(testExactMonitor) {
   BOOST_CHECK_EQUAL(status, static_cast<int>(ChimeraTK::StatusOutput::Status::OK));
 
   //set watch value different than required value
-  watch = 41.4;
+  watch = 414;
   watch.write();
   test.stepApplication();
   status.readLatest();
@@ -613,7 +631,7 @@ BOOST_AUTO_TEST_CASE(testExactMonitor) {
   status.readLatest();
   BOOST_CHECK_EQUAL(status, static_cast<int>(ChimeraTK::StatusOutput::Status::FAULT));
 
-  watch = 40.9;
+  watch = 409;
   watch.write();
   test.stepApplication();
   status.readLatest();
@@ -621,7 +639,7 @@ BOOST_AUTO_TEST_CASE(testExactMonitor) {
   BOOST_CHECK_EQUAL(status, static_cast<int>(ChimeraTK::StatusOutput::Status::OK));
 
   //set requiredValue value different than watch value
-  requiredValue = 41.3;
+  requiredValue = 413;
   requiredValue.write();
   test.stepApplication();
   status.readLatest();
@@ -629,7 +647,7 @@ BOOST_AUTO_TEST_CASE(testExactMonitor) {
   BOOST_CHECK_EQUAL(status, static_cast<int>(ChimeraTK::StatusOutput::Status::FAULT));
 
   //set requiredValue value equals to watch value
-  requiredValue = 40.9;
+  requiredValue = 409;
   requiredValue.write();
   test.stepApplication();
   status.readLatest();
@@ -637,10 +655,12 @@ BOOST_AUTO_TEST_CASE(testExactMonitor) {
   BOOST_CHECK_EQUAL(status, static_cast<int>(ChimeraTK::StatusOutput::Status::OK));
 
   // check that the tags are applied correctly
-  BOOST_CHECK_EQUAL(status, test.readScalar<int32_t>("/MyNiceMonitorCopy/Monitor/status"));
-  BOOST_CHECK_EQUAL(status, test.readScalar<int32_t>("/MonitorOutput/Monitor/status"));
-  BOOST_CHECK_EQUAL(watch, test.readScalar<float>("/MyNiceMonitorCopy/watch"));
-  BOOST_CHECK_EQUAL(requiredValue, test.readScalar<float>("/MonitorParameters/Monitor/requiredValue"));
+  BOOST_CHECK_EQUAL(status, test.readScalar<int32_t>("/MonitorOutput/output/path"));
+  BOOST_CHECK_EQUAL(requiredValue, test.readScalar<int64_t>("/MonitorParameters/parameters/requiredValue"));
+  disable = 1;
+  disable.write();
+  test.stepApplication();
+  BOOST_CHECK_EQUAL(disable, test.readScalar<int>("/MonitorParameters/parameters/disable"));
 }
 
 /*********************************************************************************************************************/