Merge pull request 'Refactor Local Pool API' (#667) from mueller/refactor-local-pool-api into development

Reviewed-on: fsfw/fsfw#667

src/fsfw/datapoollocal/LocalDataPoolManager.cpp

@@ -14,6 +14,7 @@
 #include "internal/HasLocalDpIFManagerAttorney.h"
 #include "internal/LocalPoolDataSetAttorney.h"
 
+// TODO: Get rid of this. This should be a constructor argument, not something hardcoded in any way
 object_id_t LocalDataPoolManager::defaultHkDestination = objects::PUS_SERVICE_3_HOUSEKEEPING;
 
 LocalDataPoolManager::LocalDataPoolManager(HasLocalDataPoolIF* owner, MessageQueueIF* queueToUse,
@@ -325,28 +326,32 @@ void LocalDataPoolManager::resetHkUpdateResetHelper() {
   }
 }
 
-ReturnValue_t LocalDataPoolManager::subscribeForPeriodicPacket(sid_t sid, bool enableReporting,
+ReturnValue_t LocalDataPoolManager::subscribeForRegularPeriodicPacket(
-                                                               float collectionInterval,
+    subdp::RegularHkPeriodicParams params) {
-                                                               bool isDiagnostics,
+  return subscribeForPeriodicPacket(params);
-                                                               object_id_t packetDestination) {
+}
+
+ReturnValue_t LocalDataPoolManager::subscribeForDiagPeriodicPacket(
+    subdp::DiagnosticsHkPeriodicParams params) {
+  return subscribeForPeriodicPacket(params);
+}
+
+ReturnValue_t LocalDataPoolManager::subscribeForPeriodicPacket(subdp::ParamsBase& params) {
   struct HkReceiver hkReceiver;
-  hkReceiver.dataId.sid = sid;
+  hkReceiver.dataId.sid = params.sid;
   hkReceiver.reportingType = ReportingType::PERIODIC;
   hkReceiver.dataType = DataType::DATA_SET;
-  if (packetDestination != objects::NO_OBJECT) {
+  if (params.receiver == MessageQueueIF::NO_QUEUE) {
-    auto* receivedHkIF = ObjectManager::instance()->get<AcceptsHkPacketsIF>(packetDestination);
-    if (receivedHkIF->getHkQueue() == MessageQueueIF::NO_QUEUE) {
     hkReceiver.destinationQueue = hkDestinationId;
   } else {
-      hkReceiver.destinationQueue = receivedHkIF->getHkQueue();
+    hkReceiver.destinationQueue = params.receiver;
-    }
   }
 
-  LocalPoolDataSetBase* dataSet = HasLocalDpIFManagerAttorney::getDataSetHandle(owner, sid);
+  LocalPoolDataSetBase* dataSet = HasLocalDpIFManagerAttorney::getDataSetHandle(owner, params.sid);
   if (dataSet != nullptr) {
-    LocalPoolDataSetAttorney::setReportingEnabled(*dataSet, enableReporting);
+    LocalPoolDataSetAttorney::setReportingEnabled(*dataSet, params.enableReporting);
-    LocalPoolDataSetAttorney::setDiagnostic(*dataSet, isDiagnostics);
+    LocalPoolDataSetAttorney::setDiagnostic(*dataSet, params.isDiagnostics());
-    LocalPoolDataSetAttorney::initializePeriodicHelper(*dataSet, collectionInterval,
+    LocalPoolDataSetAttorney::initializePeriodicHelper(*dataSet, params.collectionInterval,
                                                        owner->getPeriodicOperationFrequency());
   }
 
@@ -354,26 +359,30 @@ ReturnValue_t LocalDataPoolManager::subscribeForPeriodicPacket(sid_t sid, bool e
   return returnvalue::OK;
 }
 
-ReturnValue_t LocalDataPoolManager::subscribeForUpdatePacket(sid_t sid, bool reportingEnabled,
+ReturnValue_t LocalDataPoolManager::subscribeForRegularUpdatePacket(
-                                                             bool isDiagnostics,
+    subdp::RegularHkUpdateParams params) {
-                                                             object_id_t packetDestination) {
+  return subscribeForUpdatePacket(params);
+}
+ReturnValue_t LocalDataPoolManager::subscribeForDiagUpdatePacket(
+    subdp::DiagnosticsHkUpdateParams params) {
+  return subscribeForUpdatePacket(params);
+}
+
+ReturnValue_t LocalDataPoolManager::subscribeForUpdatePacket(subdp::ParamsBase& params) {
   struct HkReceiver hkReceiver;
-  hkReceiver.dataId.sid = sid;
+  hkReceiver.dataId.sid = params.sid;
   hkReceiver.reportingType = ReportingType::UPDATE_HK;
   hkReceiver.dataType = DataType::DATA_SET;
-  if (packetDestination != objects::NO_OBJECT) {
+  if (params.receiver == MessageQueueIF::NO_QUEUE) {
-    auto* receivedHkIF = ObjectManager::instance()->get<AcceptsHkPacketsIF>(packetDestination);
-    if (receivedHkIF->getHkQueue() == MessageQueueIF::NO_QUEUE) {
     hkReceiver.destinationQueue = hkDestinationId;
   } else {
-      hkReceiver.destinationQueue = receivedHkIF->getHkQueue();
+    hkReceiver.destinationQueue = params.receiver;
-    }
   }
 
-  LocalPoolDataSetBase* dataSet = HasLocalDpIFManagerAttorney::getDataSetHandle(owner, sid);
+  LocalPoolDataSetBase* dataSet = HasLocalDpIFManagerAttorney::getDataSetHandle(owner, params.sid);
   if (dataSet != nullptr) {
     LocalPoolDataSetAttorney::setReportingEnabled(*dataSet, true);
-    LocalPoolDataSetAttorney::setDiagnostic(*dataSet, isDiagnostics);
+    LocalPoolDataSetAttorney::setDiagnostic(*dataSet, params.isDiagnostics());
   }
 
   hkReceivers.push_back(hkReceiver);

src/fsfw/datapoollocal/LocalDataPoolManager.h

@@ -241,15 +241,16 @@ class LocalDataPoolManager : public ProvidesDataPoolSubscriptionIF, public Acces
   MutexIF* getMutexHandle();
 
   LocalDataPoolManager* getPoolManagerHandle() override;
-  ReturnValue_t subscribeForPeriodicPacket(
+  ReturnValue_t subscribeForRegularPeriodicPacket(subdp::RegularHkPeriodicParams params) override;
-      sid_t sid, bool enableReporting, float collectionInterval, bool isDiagnostics,
+  ReturnValue_t subscribeForDiagPeriodicPacket(subdp::DiagnosticsHkPeriodicParams params) override;
-      object_id_t packetDestination = objects::NO_OBJECT) override;
 
-  ReturnValue_t subscribeForUpdatePacket(
+  ReturnValue_t subscribeForRegularUpdatePacket(subdp::RegularHkUpdateParams params) override;
-      sid_t sid, bool reportingEnabled, bool isDiagnostics,
+  ReturnValue_t subscribeForDiagUpdatePacket(subdp::DiagnosticsHkUpdateParams params) override;
-      object_id_t packetDestination = objects::NO_OBJECT) override;
 
  protected:
+  ReturnValue_t subscribeForPeriodicPacket(subdp::ParamsBase& params);
+  ReturnValue_t subscribeForUpdatePacket(subdp::ParamsBase& params);
+
   /** Core data structure for the actual pool data */
   localpool::DataPool localPoolMap;
   /** Every housekeeping data manager has a mutex to protect access

src/fsfw/datapoollocal/ProvidesDataPoolSubscriptionIF.h

@@ -1,24 +1,90 @@
 #ifndef FSFW_DATAPOOLLOCAL_PROVIDESDATAPOOLSUBSCRIPTION_H_
 #define FSFW_DATAPOOLLOCAL_PROVIDESDATAPOOLSUBSCRIPTION_H_
 
-#include "../ipc/messageQueueDefinitions.h"
+#include "fsfw/housekeeping/AcceptsHkPacketsIF.h"
-#include "../returnvalues/returnvalue.h"
+#include "fsfw/ipc/MessageQueueIF.h"
+#include "fsfw/ipc/messageQueueDefinitions.h"
+#include "fsfw/returnvalues/returnvalue.h"
 #include "localPoolDefinitions.h"
 
+namespace subdp {
+
+struct ParamsBase {
+  ParamsBase(sid_t sid, bool enableReporting, float collectionInterval, bool diagnostics)
+      : sid(sid),
+        enableReporting(enableReporting),
+        collectionInterval(collectionInterval),
+        diagnostics(diagnostics) {}
+
+  [[nodiscard]] bool isDiagnostics() const { return diagnostics; }
+
+  sid_t sid;
+  bool enableReporting;
+  float collectionInterval;
+  MessageQueueId_t receiver = MessageQueueIF::NO_QUEUE;
+
+ protected:
+  bool diagnostics;
+};
+
+struct RegularHkPeriodicParams : public ParamsBase {
+  RegularHkPeriodicParams(sid_t sid, bool enableReporting, float collectionInterval)
+      : ParamsBase(sid, enableReporting, collectionInterval, false) {}
+};
+
+struct DiagnosticsHkPeriodicParams : public ParamsBase {
+  DiagnosticsHkPeriodicParams(sid_t sid, bool enableReporting, float collectionInterval)
+      : ParamsBase(sid, enableReporting, collectionInterval, true) {}
+};
+
+struct RegularHkUpdateParams : public ParamsBase {
+  RegularHkUpdateParams(sid_t sid, bool enableReporting)
+      : ParamsBase(sid, enableReporting, 0.0, false) {}
+};
+
+struct DiagnosticsHkUpdateParams : public ParamsBase {
+  DiagnosticsHkUpdateParams(sid_t sid, bool enableReporting)
+      : ParamsBase(sid, enableReporting, 0.0, true) {}
+};
+}  // namespace subdp
+
 class ProvidesDataPoolSubscriptionIF {
  public:
-  virtual ~ProvidesDataPoolSubscriptionIF(){};
+  virtual ~ProvidesDataPoolSubscriptionIF() = default;
-
   /**
-   * @brief   Subscribe for the generation of periodic packets.
+   * @brief   Subscribe for the generation of periodic packets. Used for regular HK packets
    * @details
    * This subscription mechanism will generally be used by the data creator
    * to generate housekeeping packets which are downlinked directly.
    * @return
    */
-  virtual ReturnValue_t subscribeForPeriodicPacket(sid_t sid, bool enableReporting,
+  virtual ReturnValue_t subscribeForRegularPeriodicPacket(
-                                                   float collectionInterval, bool isDiagnostics,
+      subdp::RegularHkPeriodicParams params) = 0;
-                                                   object_id_t packetDestination) = 0;
+  /**
+   * @brief   Subscribe for the generation of periodic packets. Used for diagnostic packets
+   * @details
+   * This subscription mechanism will generally be used by the data creator
+   * to generate housekeeping packets which are downlinked directly.
+   * @return
+   */
+  virtual ReturnValue_t subscribeForDiagPeriodicPacket(
+      subdp::DiagnosticsHkPeriodicParams params) = 0;
+
+  [[deprecated(
+      "Please use the new API which takes all arguments as one wrapper "
+      "struct")]] virtual ReturnValue_t
+  subscribeForPeriodicPacket(sid_t sid, bool enableReporting, float collectionInterval,
+                             bool isDiagnostics,
+                             object_id_t packetDestination = objects::NO_OBJECT) {
+    if (isDiagnostics) {
+      subdp::DiagnosticsHkPeriodicParams params(sid, enableReporting, collectionInterval);
+      return subscribeForDiagPeriodicPacket(params);
+    } else {
+      subdp::RegularHkPeriodicParams params(sid, enableReporting, collectionInterval);
+      return subscribeForRegularPeriodicPacket(params);
+    }
+  }
+
   /**
    * @brief   Subscribe for the  generation of packets if the dataset
    *          is marked as changed.
@@ -29,9 +95,28 @@ class ProvidesDataPoolSubscriptionIF {
    * @param packetDestination
    * @return
    */
-  virtual ReturnValue_t subscribeForUpdatePacket(sid_t sid, bool reportingEnabled,
+  virtual ReturnValue_t subscribeForRegularUpdatePacket(subdp::RegularHkUpdateParams params) = 0;
-                                                 bool isDiagnostics,
+  virtual ReturnValue_t subscribeForDiagUpdatePacket(subdp::DiagnosticsHkUpdateParams params) = 0;
-                                                 object_id_t packetDestination) = 0;
+
+  //  virtual ReturnValue_t
+  //  subscribeForUpdatePacket(sid_t sid, bool reportingEnabled, bool isDiagnostics) {
+  //    return subscribeForUpdatePacket(sid, reportingEnabled, isDiagnostics, objects::NO_OBJECT);
+  //  }
+
+  [[deprecated(
+      "Please use the new API which takes all arguments as one wrapper "
+      "struct")]] virtual ReturnValue_t
+  subscribeForUpdatePacket(sid_t sid, bool reportingEnabled, bool isDiagnostics,
+                           object_id_t packetDestination = objects::NO_OBJECT) {
+    if (isDiagnostics) {
+      subdp::DiagnosticsHkUpdateParams params(sid, reportingEnabled);
+      return subscribeForDiagUpdatePacket(params);
+    } else {
+      subdp::RegularHkUpdateParams params(sid, reportingEnabled);
+      return subscribeForRegularUpdatePacket(params);
+    }
+  }
+
   /**
    * @brief   Subscribe for a notification message which will be sent
    *          if a dataset has changed.
@@ -46,8 +131,7 @@ class ProvidesDataPoolSubscriptionIF {
    *                              Otherwise, only an notification message is sent.
    * @return
    */
-  virtual ReturnValue_t subscribeForSetUpdateMessage(const uint32_t setId,
+  virtual ReturnValue_t subscribeForSetUpdateMessage(uint32_t setId, object_id_t destinationObject,
-                                                     object_id_t destinationObject,
                                                      MessageQueueId_t targetQueueId,
                                                      bool generateSnapshot) = 0;
   /**
@@ -64,7 +148,7 @@ class ProvidesDataPoolSubscriptionIF {
    *                              only an notification message is sent.
    * @return
    */
-  virtual ReturnValue_t subscribeForVariableUpdateMessage(const lp_id_t localPoolId,
+  virtual ReturnValue_t subscribeForVariableUpdateMessage(lp_id_t localPoolId,
                                                           object_id_t destinationObject,
                                                           MessageQueueId_t targetQueueId,
                                                           bool generateSnapshot) = 0;

src/fsfw/internalerror/InternalErrorReporter.cpp

@@ -129,9 +129,9 @@ ReturnValue_t InternalErrorReporter::initializeLocalDataPool(localpool::DataPool
   localDataPoolMap.emplace(errorPoolIds::TM_HITS, &tmHitsEntry);
   localDataPoolMap.emplace(errorPoolIds::QUEUE_HITS, &queueHitsEntry);
   localDataPoolMap.emplace(errorPoolIds::STORE_HITS, &storeHitsEntry);
-  poolManager.subscribeForPeriodicPacket(
+  poolManager.subscribeForDiagPeriodicPacket(subdp::DiagnosticsHkPeriodicParams(
       internalErrorSid, false,
-      static_cast<float>(getPeriodicOperationFrequency()) / static_cast<float>(1000.0), true);
+      static_cast<float>(getPeriodicOperationFrequency()) / static_cast<float>(1000.0)));
   internalErrorDataset.setValidity(true, true);
   return returnvalue::OK;
 }

src/fsfw_hal/devicehandlers/MgmLIS3MDLHandler.cpp

@@ -466,7 +466,7 @@ ReturnValue_t MgmLIS3MDLHandler::initializeLocalDataPool(localpool::DataPool &lo
                                                          LocalDataPoolManager &poolManager) {
   localDataPoolMap.emplace(MGMLIS3MDL::FIELD_STRENGTHS, &mgmXYZ);
   localDataPoolMap.emplace(MGMLIS3MDL::TEMPERATURE_CELCIUS, &temperature);
-  poolManager.subscribeForPeriodicPacket(dataset.getSid(), false, 10.0, false);
+  poolManager.subscribeForRegularPeriodicPacket({dataset.getSid(), false, 10.0});
   return returnvalue::OK;
 }
 

src/fsfw_hal/devicehandlers/MgmRM3100Handler.cpp

@@ -310,7 +310,7 @@ void MgmRM3100Handler::modeChanged() { internalState = InternalState::NONE; }
 ReturnValue_t MgmRM3100Handler::initializeLocalDataPool(localpool::DataPool &localDataPoolMap,
                                                         LocalDataPoolManager &poolManager) {
   localDataPoolMap.emplace(RM3100::FIELD_STRENGTHS, &mgmXYZ);
-  poolManager.subscribeForPeriodicPacket(primaryDataset.getSid(), false, 10.0, false);
+  poolManager.subscribeForRegularPeriodicPacket({primaryDataset.getSid(), false, 10.0});
   return returnvalue::OK;
 }
 

unittests/mocks/LocalPoolOwnerBase.h

@@ -11,7 +11,6 @@
 #include <fsfw/objectmanager/SystemObject.h>
 
 #include "fsfw/datapool/PoolEntry.h"
-#include "fsfw/housekeeping/AcceptsHkPacketsIF.h"
 #include "mocks/MessageQueueMock.h"
 #include "tests/TestsConfig.h"
 
@@ -105,7 +104,8 @@ class LocalPoolOwnerBase : public SystemObject, public HasLocalDataPoolIF {
   }
 
   ReturnValue_t subscribePeriodicHk(bool enableReporting) {
-    return poolManager.subscribeForPeriodicPacket(lpool::testSid, enableReporting, 0.2, false);
+    return poolManager.subscribeForRegularPeriodicPacket(
+        subdp::RegularHkPeriodicParams(lpool::testSid, enableReporting, 0.2));
   }
 
   ReturnValue_t subscribeWrapperSetUpdate(MessageQueueId_t receiverId) {
@@ -121,9 +121,17 @@ class LocalPoolOwnerBase : public SystemObject, public HasLocalDataPoolIF {
   ReturnValue_t subscribeWrapperSetUpdateHk(bool diagnostics = false,
                                             AcceptsHkPacketsIF* receiver = nullptr) {
     if (diagnostics) {
-      return poolManager.subscribeForUpdatePacket(lpool::testSid, true, true);
+      auto params = subdp::DiagnosticsHkUpdateParams(lpool::testSid, true);
+      if (receiver != nullptr) {
+        params.receiver = receiver->getHkQueue();
+      }
+      return poolManager.subscribeForDiagUpdatePacket(params);
     } else {
-      return poolManager.subscribeForUpdatePacket(lpool::testSid, true, false);
+      auto params = subdp::RegularHkUpdateParams(lpool::testSid, true);
+      if (receiver != nullptr) {
+        params.receiver = receiver->getHkQueue();
+      }
+      return poolManager.subscribeForRegularUpdatePacket(params);
     }
   }