From a993c4e0d4d02b416a3e41e99a01238dec0dd9d9 Mon Sep 17 00:00:00 2001 From: Ulrich Mohr Date: Thu, 12 Jan 2023 15:40:52 +0100 Subject: [PATCH] adding linux ci and fixing problems --- automation/Jenkinsfile | 96 +++++++++++++------ src/fsfw/events/EventManager.cpp | 6 ++ src/fsfw/events/EventManager.h | 1 + src/fsfw/events/EventManagerIF.h | 1 + src/fsfw/fdir/FailureIsolationBase.cpp | 2 +- src/fsfw/globalfunctions/matching/MatchTree.h | 41 +++++++- src/fsfw/health/HealthTable.cpp | 2 - .../internalerror/InternalErrorReporter.cpp | 7 +- src/fsfw/objectmanager/ObjectManager.cpp | 18 +++- src/fsfw/objectmanager/ObjectManager.h | 10 +- src/fsfw/osal/CMakeLists.txt | 10 +- src/fsfw/osal/linux/MessageQueue.cpp | 2 +- src/fsfw/osal/osal.h.in | 36 +++++++ unittests/CatchSetup.cpp | 5 +- .../devicehandler/DeviceHandlerCommander.cpp | 4 +- unittests/osal/TestSemaphore.cpp | 68 ++++++------- 16 files changed, 230 insertions(+), 79 deletions(-) create mode 100644 src/fsfw/osal/osal.h.in diff --git a/automation/Jenkinsfile b/automation/Jenkinsfile index 27136606..64c32237 100644 --- a/automation/Jenkinsfile +++ b/automation/Jenkinsfile @@ -1,45 +1,87 @@ pipeline { environment { - BUILDDIR = 'cmake-build-tests' + BUILDDIR_HOST = 'cmake-build-tests-host' + BUILDDIR_LINUX = 'cmake-build-tests-linux' DOCDDIR = 'cmake-build-documentation' } agent { docker { image 'fsfw-ci:d6' - args '--network host' + args '--network host --sysctl fs.mqueue.msg_max=100' } } stages { - stage('Clean') { - steps { - sh 'rm -rf $BUILDDIR' - } - } - stage('Configure') { - steps { - dir(BUILDDIR) { - sh 'cmake -DFSFW_OSAL=host -DFSFW_BUILD_TESTS=ON -DFSFW_CICD_BUILD=ON ..' + stage('Host') { + stages{ + stage('Clean') { + steps { + sh 'rm -rf $BUILDDIR_HOST' + } + } + stage('Configure') { + steps { + dir(BUILDDIR_HOST) { + sh 'cmake -DFSFW_OSAL=host -DFSFW_BUILD_TESTS=ON -DFSFW_CICD_BUILD=ON ..' + } + } + } + stage('Build') { + steps { + dir(BUILDDIR_HOST) { + sh 'cmake --build . -j4' + } + } + } + stage('Unittests') { + steps { + dir(BUILDDIR_HOST) { + sh 'cmake --build . -- fsfw-tests_coverage -j4' + } + } + } + stage('Valgrind') { + steps { + dir(BUILDDIR_HOST) { + sh 'valgrind --leak-check=full --error-exitcode=1 ./fsfw-tests' + } + } } } } - stage('Build') { - steps { - dir(BUILDDIR) { - sh 'cmake --build . -j4' + stage('Linux') { + stages{ + stage('Clean') { + steps { + sh 'rm -rf $BUILDDIR_LINUX' + } } - } - } - stage('Unittests') { - steps { - dir(BUILDDIR) { - sh 'cmake --build . -- fsfw-tests_coverage -j4' + stage('Configure') { + steps { + dir(BUILDDIR_LINUX) { + sh 'cmake -DFSFW_OSAL=linux -DFSFW_BUILD_TESTS=ON -DFSFW_CICD_BUILD=ON ..' + } + } } - } - } - stage('Valgrind') { - steps { - dir(BUILDDIR) { - sh 'valgrind --leak-check=full --error-exitcode=1 ./fsfw-tests' + stage('Build') { + steps { + dir(BUILDDIR_LINUX) { + sh 'cmake --build . -j4' + } + } + } + stage('Unittests') { + steps { + dir(BUILDDIR_LINUX) { + sh 'cmake --build . -- fsfw-tests_coverage -j4' + } + } + } + stage('Valgrind') { + steps { + dir(BUILDDIR_LINUX) { + sh 'valgrind --leak-check=full --error-exitcode=1 ./fsfw-tests' + } + } } } } diff --git a/src/fsfw/events/EventManager.cpp b/src/fsfw/events/EventManager.cpp index f4466817..8bd26282 100644 --- a/src/fsfw/events/EventManager.cpp +++ b/src/fsfw/events/EventManager.cpp @@ -23,6 +23,7 @@ EventManager::EventManager(object_id_t setObjectId) } EventManager::~EventManager() { + listenerList.clear(); QueueFactory::instance()->deleteMessageQueue(eventReportQueue); MutexFactory::instance()->deleteMutex(mutex); } @@ -61,9 +62,14 @@ ReturnValue_t EventManager::registerListener(MessageQueueId_t listener, if (!result.second) { return returnvalue::FAILED; } + return returnvalue::OK; } +ReturnValue_t EventManager::unregisterListener(MessageQueueId_t listener) { + return listenerList.erase(listener) == 1 ? returnvalue::OK : returnvalue::FAILED; +} + ReturnValue_t EventManager::subscribeToEvent(MessageQueueId_t listener, EventId_t event) { return subscribeToEventRange(listener, event); } diff --git a/src/fsfw/events/EventManager.h b/src/fsfw/events/EventManager.h index 70245f5d..0e6dace0 100644 --- a/src/fsfw/events/EventManager.h +++ b/src/fsfw/events/EventManager.h @@ -31,6 +31,7 @@ class EventManager : public EventManagerIF, public ExecutableObjectIF, public Sy MessageQueueId_t getEventReportQueue(); ReturnValue_t registerListener(MessageQueueId_t listener, bool forwardAllButSelected = false); + ReturnValue_t unregisterListener(MessageQueueId_t listener) override; ReturnValue_t subscribeToEvent(MessageQueueId_t listener, EventId_t event); ReturnValue_t subscribeToAllEventsFrom(MessageQueueId_t listener, object_id_t object); ReturnValue_t subscribeToEventRange(MessageQueueId_t listener, EventId_t idFrom = 0, diff --git a/src/fsfw/events/EventManagerIF.h b/src/fsfw/events/EventManagerIF.h index adb61f2b..12014090 100644 --- a/src/fsfw/events/EventManagerIF.h +++ b/src/fsfw/events/EventManagerIF.h @@ -18,6 +18,7 @@ class EventManagerIF { virtual ReturnValue_t registerListener(MessageQueueId_t listener, bool forwardAllButSelected = false) = 0; + virtual ReturnValue_t unregisterListener(MessageQueueId_t listener) = 0; virtual ReturnValue_t subscribeToEvent(MessageQueueId_t listener, EventId_t event) = 0; virtual ReturnValue_t subscribeToAllEventsFrom(MessageQueueId_t listener, object_id_t object) = 0; virtual ReturnValue_t unsubscribeFromAllEvents(MessageQueueId_t listener, object_id_t object) = 0; diff --git a/src/fsfw/fdir/FailureIsolationBase.cpp b/src/fsfw/fdir/FailureIsolationBase.cpp index 28df16d8..0d3b0214 100644 --- a/src/fsfw/fdir/FailureIsolationBase.cpp +++ b/src/fsfw/fdir/FailureIsolationBase.cpp @@ -23,7 +23,7 @@ FailureIsolationBase::~FailureIsolationBase() { #endif return; } - manager->unsubscribeFromAllEvents(eventQueue->getId(), ownerId); + manager->unregisterListener(eventQueue->getId()); QueueFactory::instance()->deleteMessageQueue(eventQueue); } diff --git a/src/fsfw/globalfunctions/matching/MatchTree.h b/src/fsfw/globalfunctions/matching/MatchTree.h index 0c31cf2a..69f660d3 100644 --- a/src/fsfw/globalfunctions/matching/MatchTree.h +++ b/src/fsfw/globalfunctions/matching/MatchTree.h @@ -24,7 +24,7 @@ class MatchTree : public SerializeableMatcherIF, public BinaryTree>(root.element), maxDepth(maxDepth) {} MatchTree() : BinaryTree>(), maxDepth(-1) {} - virtual ~MatchTree() {} + virtual ~MatchTree() { clear(); } virtual bool match(T number) override { return matchesTree(number); } bool matchesTree(T number) { iterator iter = this->begin(); @@ -176,6 +176,45 @@ class MatchTree : public SerializeableMatcherIF, public BinaryTree>::rootNode; + + if (localRoot == nullptr) { + return; + } + + Node* node = localRoot->left; + + while (true) { + if (node->left != nullptr) { + node = node->left; + continue; + } + if (node->right != nullptr) { + node = node->right; + continue; + } + if (node->parent == nullptr) { + // this is the root node with no children + if (node->value != nullptr) { + cleanUpElement(iterator(node)); + } + return; + } + // leaf + { + Node* parent = node->parent; + if (parent->left == node) { + parent->left = nullptr; + } else { + parent->right = nullptr; + } + cleanUpElement(iterator(node)); + node = parent; + } + } + } + virtual ReturnValue_t cleanUpElement(iterator position) { return returnvalue::OK; } bool matchSubtree(iterator iter, T number) { diff --git a/src/fsfw/health/HealthTable.cpp b/src/fsfw/health/HealthTable.cpp index 5fb45fb9..3fcf7f77 100644 --- a/src/fsfw/health/HealthTable.cpp +++ b/src/fsfw/health/HealthTable.cpp @@ -6,8 +6,6 @@ HealthTable::HealthTable(object_id_t objectid) : SystemObject(objectid) { mutex = MutexFactory::instance()->createMutex(); - ; - mapIterator = healthMap.begin(); } diff --git a/src/fsfw/internalerror/InternalErrorReporter.cpp b/src/fsfw/internalerror/InternalErrorReporter.cpp index b3b77366..fb2dc8a6 100644 --- a/src/fsfw/internalerror/InternalErrorReporter.cpp +++ b/src/fsfw/internalerror/InternalErrorReporter.cpp @@ -7,14 +7,17 @@ InternalErrorReporter::InternalErrorReporter(object_id_t setObjectId, uint32_t messageQueueDepth) : SystemObject(setObjectId), - commandQueue(QueueFactory::instance()->createMessageQueue(messageQueueDepth)), poolManager(this, commandQueue), internalErrorSid(setObjectId, InternalErrorDataset::ERROR_SET_ID), internalErrorDataset(this) { + commandQueue = QueueFactory::instance()->createMessageQueue(messageQueueDepth); mutex = MutexFactory::instance()->createMutex(); } -InternalErrorReporter::~InternalErrorReporter() { MutexFactory::instance()->deleteMutex(mutex); } +InternalErrorReporter::~InternalErrorReporter() { + MutexFactory::instance()->deleteMutex(mutex); + QueueFactory::instance()->deleteMessageQueue(commandQueue); +} void InternalErrorReporter::setDiagnosticPrintout(bool enable) { this->diagnosticPrintout = enable; diff --git a/src/fsfw/objectmanager/ObjectManager.cpp b/src/fsfw/objectmanager/ObjectManager.cpp index ddf5ab80..29c1b2fc 100644 --- a/src/fsfw/objectmanager/ObjectManager.cpp +++ b/src/fsfw/objectmanager/ObjectManager.cpp @@ -23,9 +23,17 @@ void ObjectManager::setObjectFactoryFunction(produce_function_t objFactoryFunc, ObjectManager::ObjectManager() = default; +void ObjectManager::clear() { + if (objManagerInstance != nullptr) { + delete objManagerInstance; + objManagerInstance = nullptr; + } +} + ObjectManager::~ObjectManager() { - for (auto const& iter : objectList) { - delete iter.second; + teardown = true; + for (auto iter = objectList.begin(); iter != objectList.end(); iter = objectList.erase(iter)) { + delete iter->second; } } @@ -53,6 +61,12 @@ ReturnValue_t ObjectManager::insert(object_id_t id, SystemObjectIF* object) { } ReturnValue_t ObjectManager::remove(object_id_t id) { + // this function is called during destruction of System Objects + // disabeld for teardown to avoid iterator invalidation and + // double free + if (teardown) { + return returnvalue::OK; + } if (this->getSystemObject(id) != nullptr) { this->objectList.erase(id); #if FSFW_CPP_OSTREAM_ENABLED == 1 diff --git a/src/fsfw/objectmanager/ObjectManager.h b/src/fsfw/objectmanager/ObjectManager.h index e0e74792..355f5d0a 100644 --- a/src/fsfw/objectmanager/ObjectManager.h +++ b/src/fsfw/objectmanager/ObjectManager.h @@ -24,12 +24,17 @@ class ObjectManager : public ObjectManagerIF { using produce_function_t = void (*)(void* args); /** - * Returns the single instance of TaskFactory. + * Returns the single instance of ObjectManager. * The implementation of #instance is found in its subclasses. * Thus, we choose link-time variability of the instance. */ static ObjectManager* instance(); + /** + * Deletes the single instance of ObjectManager + */ + static void clear(); + void setObjectFactoryFunction(produce_function_t prodFunc, void* args); template @@ -66,6 +71,9 @@ class ObjectManager : public ObjectManagerIF { */ std::map objectList; static ObjectManager* objManagerInstance; + // used when the OM itself is deleted to modify behaviour of remove() + // to avoid iterator invalidation and double free + bool teardown = false; }; // Documentation can be found in the class method declaration above diff --git a/src/fsfw/osal/CMakeLists.txt b/src/fsfw/osal/CMakeLists.txt index 50fd6102..a4097649 100644 --- a/src/fsfw/osal/CMakeLists.txt +++ b/src/fsfw/osal/CMakeLists.txt @@ -1,10 +1,13 @@ # Check the OS_FSFW variable if(FSFW_OSAL MATCHES "freertos") add_subdirectory(freertos) + set(FSFW_OSAL_FREERTOS 1) elseif(FSFW_OSAL MATCHES "rtems") add_subdirectory(rtems) + set(FSFW_OSAL_RTEMS 1) elseif(FSFW_OSAL MATCHES "linux") add_subdirectory(linux) + set(FSFW_OSAL_LINUX 1) elseif(FSFW_OSAL MATCHES "host") add_subdirectory(host) if(WIN32) @@ -13,16 +16,17 @@ elseif(FSFW_OSAL MATCHES "host") # We still need to pull in some Linux specific sources target_sources(${LIB_FSFW_NAME} PUBLIC linux/tcpipHelpers.cpp) endif() - + set(FSFW_OSAL_HOST 1) else() - message(WARNING "The OS_FSFW variable was not set. Assuming host OS..") # Not set. Assumuing this is a host build, try to determine host OS if(WIN32) add_subdirectory(host) add_subdirectory(windows) + set(FSFW_OSAL_HOST 1) elseif(UNIX) add_subdirectory(linux) + set(FSFW_OSAL_LINUX 1) else() # MacOS or other OSes have not been tested yet / are not supported. message(FATAL_ERROR "The host OS could not be determined! Aborting.") @@ -31,3 +35,5 @@ else() endif() add_subdirectory(common) + +configure_file(osal.h.in ${CMAKE_BINARY_DIR}/fsfw/osal/osal.h) diff --git a/src/fsfw/osal/linux/MessageQueue.cpp b/src/fsfw/osal/linux/MessageQueue.cpp index b2cca3f1..c08e3775 100644 --- a/src/fsfw/osal/linux/MessageQueue.cpp +++ b/src/fsfw/osal/linux/MessageQueue.cpp @@ -21,7 +21,7 @@ MessageQueue::MessageQueue(uint32_t messageDepth, size_t maxMessageSize, MqArgs* attributes.mq_msgsize = maxMessageSize; attributes.mq_flags = 0; // Flags are ignored on Linux during mq_open // Set the name of the queue. The slash is mandatory! - sprintf(name, "/FSFW_MQ%u\n", queueCounter++); + sprintf(name, "/FSFW_MQ%u", queueCounter++); // Create a nonblocking queue if the name is available (the queue is read // and writable for the owner as well as the group) diff --git a/src/fsfw/osal/osal.h.in b/src/fsfw/osal/osal.h.in new file mode 100644 index 00000000..8e48c31a --- /dev/null +++ b/src/fsfw/osal/osal.h.in @@ -0,0 +1,36 @@ +#pragma once + +namespace osal { + enum osalTarget{ + HOST, + LINUX, + WINDOWS, + FREERTOS, + RTEMS, + }; + +#cmakedefine FSFW_OSAL_HOST +#cmakedefine FSFW_OSAL_LINUX +#cmakedefine FSFW_OSAL_WINDOWS +#cmakedefine FSFW_OSAL_FREERTOS +#cmakedefine FSFW_OSAL_RTEMS + + + constexpr osalTarget getTarget() { +#ifdef FSFW_OSAL_HOST + return HOST; +#endif +#ifdef FSFW_OSAL_LINUX + return LINUX; +#endif +#ifdef FSFW_OSAL_WINDOWS + return WINDOWS; +#endif +#ifdef FSFW_OSAL_FREERTOS + return FREERTOS; +#endif +#ifdef FSFW_OSAL_RTEMS + return RTEMS; +#endif + } +}; \ No newline at end of file diff --git a/unittests/CatchSetup.cpp b/unittests/CatchSetup.cpp index 4f2a4a54..fc5bb3f5 100644 --- a/unittests/CatchSetup.cpp +++ b/unittests/CatchSetup.cpp @@ -31,4 +31,7 @@ int customSetup() { return 0; } -int customTeardown() { return 0; } +int customTeardown() { + ObjectManager::clear(); + return 0; +} diff --git a/unittests/devicehandler/DeviceHandlerCommander.cpp b/unittests/devicehandler/DeviceHandlerCommander.cpp index 03ff992c..835faf2b 100644 --- a/unittests/devicehandler/DeviceHandlerCommander.cpp +++ b/unittests/devicehandler/DeviceHandlerCommander.cpp @@ -9,7 +9,9 @@ DeviceHandlerCommander::DeviceHandlerCommander(object_id_t objectId) QUEUE_SIZE, MessageQueueMessage::MAX_MESSAGE_SIZE, &mqArgs); } -DeviceHandlerCommander::~DeviceHandlerCommander() {} +DeviceHandlerCommander::~DeviceHandlerCommander() { + QueueFactory::instance()->deleteMessageQueue(commandQueue); +} ReturnValue_t DeviceHandlerCommander::performOperation(uint8_t operationCode) { readCommandQueue(); diff --git a/unittests/osal/TestSemaphore.cpp b/unittests/osal/TestSemaphore.cpp index 376b08db..1d8758b1 100644 --- a/unittests/osal/TestSemaphore.cpp +++ b/unittests/osal/TestSemaphore.cpp @@ -1,46 +1,38 @@ - -#ifdef LINUX - -/* +#include #include #include -#include "catch.hpp" -#include "core/CatchDefinitions.h" +#include -TEST_CASE("Binary Semaphore Test" , "[BinSemaphore]") { - //perform set-up here - SemaphoreIF* binSemaph = SemaphoreFactory::instance()-> - createBinarySemaphore(); - REQUIRE(binSemaph != nullptr); - SECTION("Simple Test") { - // set-up is run for each section - REQUIRE(binSemaph->getSemaphoreCounter() == 1); - REQUIRE(binSemaph->release() == - static_cast(SemaphoreIF::SEMAPHORE_NOT_OWNED)); - REQUIRE(binSemaph->acquire(SemaphoreIF::POLLING) == - result::OK); - { - // not precise enough on linux.. should use clock instead.. - //Stopwatch stopwatch(false); - //REQUIRE(binSemaph->acquire(SemaphoreIF::TimeoutType::WAITING, 5) == - // SemaphoreIF::SEMAPHORE_TIMEOUT); - //dur_millis_t time = stopwatch.stop(); - //CHECK(time == 5); - } - REQUIRE(binSemaph->getSemaphoreCounter() == 0); - REQUIRE(binSemaph->release() == result::OK); - } - SemaphoreFactory::instance()->deleteSemaphore(binSemaph); - // perform tear-down here +// binary semaphores currently only supported on linux +#ifdef FSFW_OSAL_LINUX + +TEST_CASE("Binary Semaphore Test", "[BinSemaphore]") { + // perform set-up here + SemaphoreIF* binSemaph = SemaphoreFactory::instance()->createBinarySemaphore(); + REQUIRE(binSemaph != nullptr); + SECTION("Simple Test") { + // set-up is run for each section + REQUIRE(binSemaph->getSemaphoreCounter() == 1); + REQUIRE(binSemaph->release() == static_cast(SemaphoreIF::SEMAPHORE_NOT_OWNED)); + REQUIRE(binSemaph->acquire(SemaphoreIF::POLLING) == returnvalue::OK); + { + // not precise enough on linux.. should use clock instead.. + // Stopwatch stopwatch(false); + // REQUIRE(binSemaph->acquire(SemaphoreIF::TimeoutType::WAITING, 5) == + // SemaphoreIF::SEMAPHORE_TIMEOUT); + // dur_millis_t time = stopwatch.stop(); + // CHECK(time == 5); + } + REQUIRE(binSemaph->getSemaphoreCounter() == 0); + REQUIRE(binSemaph->release() == returnvalue::OK); + } + SemaphoreFactory::instance()->deleteSemaphore(binSemaph); + // perform tear-down here } - -TEST_CASE("Counting Semaphore Test" , "[CountingSemaph]") { - SECTION("Simple Test") { - - } +TEST_CASE("Counting Semaphore Test", "[CountingSemaph]") { + SECTION("Simple Test") {} } -*/ -#endif +#endif \ No newline at end of file