Merge remote-tracking branch 'origin/development' into mueller/clang-improvements

CHANGELOG.md

@@ -46,15 +46,33 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## Additions
 
+- LTO support: Allow using LTO/IPO by setting `FSFW_ENABLE_LTO=1`. CMake is able to detect whether
+  the user compiler supports IPO/LPO. LTO is on by default now. Most modern compilers support it,
+  can make good use of it and it usually makes the code faster and/or smaller.
+  After some more research:
+  Enabling LTO will actually cause the compiler to only produce thin LTO by adding 
+  `-flto -fno-fat-lto-objects` to the compiler options. I am not sure this is an ideal choice
+  because if an application linking against the FSFW does not use LTO, there can be compile
+  issues (e.g. observed when compiling the FSFW tests without LTO). This is a known issue as
+  can be seen in the multiple CMake issues for it: 
+    - https://gitlab.kitware.com/cmake/cmake/-/issues/22913, 
+    - https://gitlab.kitware.com/cmake/cmake/-/issues/16808, 
+    - https://gitlab.kitware.com/cmake/cmake/-/issues/21696
+  Easiest solution for now: Keep this option OFF by default.
+  PR: https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/616
 - Linux HAL: Add wiretapping option for I2C. Enabled with `FSFW_HAL_I2C_WIRETAPPING` defined to 1
 - Dedicated Version class and constant `fsfw::FSFW_VERSION` containing version information
   inside `fsfw/version.h`
   PR: https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/559
 - Added ETL dependency and improved library dependency management
   PR: https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/592
+- `Subsystem`: New API to add table and sequence entries
 
 ## Fixed
 
+- TCP TMTC Server: `MutexGuard` was not created properly in
+  `TcpTmTcServer::handleTmSending(socket_t connSocket, bool& tmSent)` call.
+  PR: https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/618
 - Fix infinite recursion in `prepareHealthSetReply` of PUS Health Service 201.
   Is not currently used right now but might be used in the future
   PR: https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/617

CMakeLists.txt

@@ -17,11 +17,12 @@ set(FSFW_REVISION 0)
 # Add the cmake folder so the FindSphinx module is found
 set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake" ${CMAKE_MODULE_PATH})
 
+set(FSFW_ETL_LIB_NAME etl)
 set(FSFW_ETL_LIB_MAJOR_VERSION 20 CACHE STRING
-    "ETL library major version requirement"
+  "ETL library major version requirement"
 )
 set(FSFW_ETL_LIB_VERSION ${FSFW_ETL_LIB_MAJOR_VERSION}.27.3 CACHE STRING
-    "ETL library exact version requirement"
+  "ETL library exact version requirement"
 )
 set(FSFW_ETL_LINK_TARGET etl::etl)
 
@@ -32,19 +33,28 @@ set(FSFW_CATCH2_LIB_VERSION v${FSFW_CATCH2_LIB_MAJOR_VERSION}.0.0-preview5 CACHE
     "Catch2 library exact version requirement"
 )
 
-set(FSFW_ETL_LIB_NAME etl)
+# Keep this off by default for now. See PR: https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/616
+# for information which keeping this on by default is problematic
+option(FSFW_ENABLE_IPO "Enable interprocedural optimization or link-time optimization if available" OFF)
+if(FSFW_ENABLE_IPO)
+  include(CheckIPOSupported)
+  check_ipo_supported(RESULT IPO_SUPPORTED OUTPUT IPO_ERROR)
+  if(NOT IPO_SUPPORTED)
+    message(STATUS "FSFW | IPO/LTO not supported: ${IPO_ERROR}")
+  endif()
+endif()
 
 option(FSFW_GENERATE_SECTIONS
-    "Generate function and data sections. Required to remove unused code" ON
+  "Generate function and data sections. Required to remove unused code" ON
 )
 if(FSFW_GENERATE_SECTIONS)
-    option(FSFW_REMOVE_UNUSED_CODE "Remove unused code" ON)
+  option(FSFW_REMOVE_UNUSED_CODE "Remove unused code" ON)
 endif()
 
 option(FSFW_BUILD_UNITTESTS "Build unittest binary in addition to static library" OFF)
 option(FSFW_BUILD_DOCS "Build documentation with Sphinx and Doxygen" OFF)
 if(FSFW_BUILD_UNITTESTS)
-    option(FSFW_TESTS_GEN_COV "Generate coverage data for unittests" ON)
+  option(FSFW_TESTS_GEN_COV "Generate coverage data for unittests" ON)
 endif()
 
 option(FSFW_WARNING_SHADOW_LOCAL_GCC "Enable -Wshadow=local warning in GCC" ON)
@@ -71,6 +81,10 @@ set(FSFW_DUMMY_TGT fsfw-dummy)
 
 add_library(${LIB_FSFW_NAME})
 
+if(IPO_SUPPORTED AND FSFW_ENABLE_IPO)
+  set_property(TARGET ${LIB_FSFW_NAME} PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE)
+endif()
+
 if(FSFW_BUILD_UNITTESTS)
     message(STATUS "Building the FSFW unittests in addition to the static library")
     # Check whether the user has already installed Catch2 first
@@ -95,6 +109,9 @@ if(FSFW_BUILD_UNITTESTS)
 
     project(${FSFW_TEST_TGT} CXX C)
     add_executable(${FSFW_TEST_TGT})
+    if(IPO_SUPPORTED AND FSFW_ENABLE_IPO)
+      set_property(TARGET ${FSFW_TEST_TGT} PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE)
+    endif()
 
     if(FSFW_TESTS_GEN_COV)
         message(STATUS "Generating coverage data for the library")

src/fsfw/container/FixedArrayList.h

@@ -2,6 +2,7 @@
 #define FIXEDARRAYLIST_H_
 
 #include <cmath>
+#include <limits>
 
 #include "ArrayList.h"
 /**
@@ -9,10 +10,8 @@
  */
 template <typename T, size_t MAX_SIZE, typename count_t = uint8_t>
 class FixedArrayList : public ArrayList<T, count_t> {
-#if !defined(_MSC_VER) && !defined(__clang__)
-  static_assert(MAX_SIZE <= (std::pow(2, sizeof(count_t) * 8) - 1),
+  static_assert(MAX_SIZE <= std::numeric_limits<count_t>::max(),
                 "count_t is not large enough to hold MAX_SIZE");
-#endif
  private:
   T data[MAX_SIZE];
 

src/fsfw/osal/common/TcpTmTcServer.cpp

@@ -105,7 +105,7 @@ ReturnValue_t TcpTmTcServer::initialize() {
 
 TcpTmTcServer::~TcpTmTcServer() { closeSocket(listenerTcpSocket); }
 
-ReturnValue_t TcpTmTcServer::performOperation(uint8_t opCode) {
+[[noreturn]] ReturnValue_t TcpTmTcServer::performOperation(uint8_t opCode) {
   using namespace tcpip;
   // If a connection is accepted, the corresponding socket will be assigned to the new socket
   socket_t connSocket = 0;
@@ -140,7 +140,6 @@ ReturnValue_t TcpTmTcServer::performOperation(uint8_t opCode) {
     closeSocket(connSocket);
     connSocket = 0;
   }
-  return HasReturnvaluesIF::RETURN_OK;
 }
 
 ReturnValue_t TcpTmTcServer::initializeAfterTaskCreation() {
@@ -161,7 +160,7 @@ void TcpTmTcServer::handleServerOperation(socket_t& connSocket) {
 #endif
 
   while (true) {
-    int retval = recv(connSocket, reinterpret_cast<char*>(receptionBuffer.data()),
+    ssize_t retval = recv(connSocket, reinterpret_cast<char*>(receptionBuffer.data()),
                       receptionBuffer.capacity(), tcpConfig.tcpFlags);
     if (retval == 0) {
       size_t availableReadData = ringBuffer.getAvailableReadData();
@@ -254,17 +253,17 @@ ReturnValue_t TcpTmTcServer::handleTcReception(uint8_t* spacePacket, size_t pack
   return result;
 }
 
-std::string TcpTmTcServer::getTcpPort() const { return tcpConfig.tcpPort; }
+const std::string& TcpTmTcServer::getTcpPort() const { return tcpConfig.tcpPort; }
 
-void TcpTmTcServer::setSpacePacketParsingOptions(std::vector<uint16_t> validPacketIds) {
-  this->validPacketIds = validPacketIds;
+void TcpTmTcServer::setSpacePacketParsingOptions(std::vector<uint16_t> validPacketIds_) {
+  this->validPacketIds = std::move(validPacketIds_);
 }
 
 TcpTmTcServer::TcpConfig& TcpTmTcServer::getTcpConfigStruct() { return tcpConfig; }
 
 ReturnValue_t TcpTmTcServer::handleTmSending(socket_t connSocket, bool& tmSent) {
   // Access to the FIFO is mutex protected because it is filled by the bridge
-  MutexGuard(tmtcBridge->mutex, tmtcBridge->timeoutType, tmtcBridge->mutexTimeoutMs);
+  MutexGuard mg(tmtcBridge->mutex, tmtcBridge->timeoutType, tmtcBridge->mutexTimeoutMs);
   store_address_t storeId;
   while ((not tmtcBridge->tmFifo->empty()) and
          (tmtcBridge->packetSentCounter < tmtcBridge->sentPacketsPerCycle)) {
@@ -285,7 +284,7 @@ ReturnValue_t TcpTmTcServer::handleTmSending(socket_t connSocket, bool& tmSent)
 #endif
       arrayprinter::print(storeAccessor.data(), storeAccessor.size());
     }
-    int retval = send(connSocket, reinterpret_cast<const char*>(storeAccessor.data()),
+    ssize_t retval = send(connSocket, reinterpret_cast<const char*>(storeAccessor.data()),
                       storeAccessor.size(), tcpConfig.tcpTmFlags);
     if (retval == static_cast<int>(storeAccessor.size())) {
       // Packet sent, clear FIFO entry
@@ -341,6 +340,9 @@ ReturnValue_t TcpTmTcServer::handleTcRingBufferData(size_t availableReadData) {
   size_t foundSize = 0;
   size_t readLen = 0;
   while (readLen < readAmount) {
+    if(spacePacketParser == nullptr) {
+      return HasReturnvaluesIF::RETURN_FAILED;
+    }
     result =
         spacePacketParser->parseSpacePackets(bufPtrPtr, readAmount, startIdx, foundSize, readLen);
     switch (result) {

src/fsfw/osal/common/TcpTmTcServer.h

@@ -45,7 +45,7 @@ class TcpTmTcServer : public SystemObject, public TcpIpBase, public ExecutableOb
 
   struct TcpConfig {
    public:
-    TcpConfig(std::string tcpPort) : tcpPort(std::move(tcpPort)) {}
+    explicit TcpConfig(std::string tcpPort) : tcpPort(std::move(tcpPort)) {}
 
     /**
      * Passed to the recv call
@@ -98,10 +98,10 @@ class TcpTmTcServer : public SystemObject, public TcpIpBase, public ExecutableOb
   void setSpacePacketParsingOptions(std::vector<uint16_t> validPacketIds);
 
   ReturnValue_t initialize() override;
-  ReturnValue_t performOperation(uint8_t opCode) override;
+  [[noreturn]] ReturnValue_t performOperation(uint8_t opCode) override;
   ReturnValue_t initializeAfterTaskCreation() override;
 
-  std::string getTcpPort() const;
+  [[nodiscard]] const std::string& getTcpPort() const;
 
  protected:
   StorageManagerIF* tcStore = nullptr;
@@ -116,7 +116,7 @@ class TcpTmTcServer : public SystemObject, public TcpIpBase, public ExecutableOb
 
   ReceptionModes receptionMode;
   TcpConfig tcpConfig;
-  struct sockaddr tcpAddress;
+  struct sockaddr tcpAddress = {};
   socket_t listenerTcpSocket = 0;
 
   MessageQueueId_t targetTcDestination = MessageQueueIF::NO_QUEUE;

src/fsfw/subsystem/Subsystem.cpp

@@ -307,6 +307,11 @@ void Subsystem::replyToCommand(ReturnValue_t status, uint32_t parameter) {
   }
 }
 
+ReturnValue_t Subsystem::addSequence(SequenceEntry sequence) {
+  return addSequence(sequence.table, sequence.mode, sequence.fallbackMode, sequence.inStore,
+                     sequence.preInit);
+}
+
 ReturnValue_t Subsystem::addSequence(ArrayList<ModeListEntry> *sequence, Mode_t id,
                                      Mode_t fallbackSequence, bool inStore, bool preInit) {
   ReturnValue_t result;
@@ -350,6 +355,10 @@ ReturnValue_t Subsystem::addSequence(ArrayList<ModeListEntry> *sequence, Mode_t
   return result;
 }
 
+ReturnValue_t Subsystem::addTable(TableEntry table) {
+  return addTable(table.table, table.mode, table.inStore, table.preInit);
+}
+
 ReturnValue_t Subsystem::addTable(ArrayList<ModeListEntry> *table, Mode_t id, bool inStore,
                                   bool preInit) {
   ReturnValue_t result;
@@ -458,6 +467,7 @@ ReturnValue_t Subsystem::initialize() {
   }
 
   mode = initialMode;
+  submode = initSubmode;
 
   return RETURN_OK;
 }
@@ -595,7 +605,10 @@ ReturnValue_t Subsystem::checkObjectConnections() {
   return RETURN_OK;
 }
 
-void Subsystem::setInitialMode(Mode_t mode) { initialMode = mode; }
+void Subsystem::setInitialMode(Mode_t mode, Submode_t submode) {
+  this->initialMode = mode;
+  this->initSubmode = submode;
+}
 
 void Subsystem::cantKeepMode() {
   ReturnValue_t result;

src/fsfw/subsystem/Subsystem.h

@@ -10,6 +10,28 @@
 #include "fsfw/FSFW.h"
 #include "modes/ModeDefinitions.h"
 
+struct TableSequenceBase {
+ public:
+  TableSequenceBase(Mode_t mode, ArrayList<ModeListEntry> *table) : mode(mode), table(table){};
+  Mode_t mode;
+  ArrayList<ModeListEntry> *table;
+  bool inStore = false;
+  bool preInit = true;
+};
+
+struct TableEntry : public TableSequenceBase {
+ public:
+  TableEntry(Mode_t mode, ArrayList<ModeListEntry> *table) : TableSequenceBase(mode, table){};
+};
+
+struct SequenceEntry : public TableSequenceBase {
+ public:
+  SequenceEntry(Mode_t mode, ArrayList<ModeListEntry> *table, Mode_t fallbackMode)
+      : TableSequenceBase(mode, table), fallbackMode(fallbackMode) {}
+
+  Mode_t fallbackMode;
+};
+
 /**
  * @brief   TODO: documentation missing
  * @details
@@ -44,13 +66,15 @@ class Subsystem : public SubsystemBase, public HasModeSequenceIF {
             uint32_t maxNumberOfTables);
   virtual ~Subsystem();
 
+  ReturnValue_t addSequence(SequenceEntry sequence);
   ReturnValue_t addSequence(ArrayList<ModeListEntry> *sequence, Mode_t id, Mode_t fallbackSequence,
                             bool inStore = true, bool preInit = true);
 
+  ReturnValue_t addTable(TableEntry table);
   ReturnValue_t addTable(ArrayList<ModeListEntry> *table, Mode_t id, bool inStore = true,
                          bool preInit = true);
 
-  void setInitialMode(Mode_t mode);
+  void setInitialMode(Mode_t mode, Submode_t submode = SUBMODE_NONE);
 
   virtual ReturnValue_t initialize() override;
 
@@ -89,6 +113,7 @@ class Subsystem : public SubsystemBase, public HasModeSequenceIF {
   Submode_t targetSubmode;
 
   Mode_t initialMode = 0;
+  Submode_t initSubmode = SUBMODE_NONE;
 
   HybridIterator<ModeListEntry> currentSequenceIterator;