From 2745b2080dc9c53ef6699c844ba1d000351d07ba Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 7 Mar 2023 13:55:40 +0100 Subject: [PATCH 1/4] allow submode mask now --- src/fsfw/osal/host/Clock.cpp | 4 +- src/fsfw/osal/linux/Clock.cpp | 8 +- src/fsfw/pus/CServiceHealthCommanding.cpp | 2 +- src/fsfw/subsystem/SubsystemBase.cpp | 26 ++-- src/fsfw/subsystem/modes/ModeDefinitions.h | 155 ++++++++++----------- unittests/CMakeLists.txt | 1 + unittests/subsystem/CMakeLists.txt | 1 + unittests/subsystem/testModeDef.cpp | 49 +++++++ 8 files changed, 147 insertions(+), 99 deletions(-) create mode 100644 unittests/subsystem/CMakeLists.txt create mode 100644 unittests/subsystem/testModeDef.cpp diff --git a/src/fsfw/osal/host/Clock.cpp b/src/fsfw/osal/host/Clock.cpp index ec66a11d..2a3c94bc 100644 --- a/src/fsfw/osal/host/Clock.cpp +++ b/src/fsfw/osal/host/Clock.cpp @@ -100,9 +100,7 @@ ReturnValue_t Clock::getClock(timeval* time) { #endif } -ReturnValue_t Clock::getClock_timeval(timeval* time) { - return Clock::getClock(time); -} +ReturnValue_t Clock::getClock_timeval(timeval* time) { return Clock::getClock(time); } ReturnValue_t Clock::getClock_usecs(uint64_t* time) { if (time == nullptr) { diff --git a/src/fsfw/osal/linux/Clock.cpp b/src/fsfw/osal/linux/Clock.cpp index 050ec099..fd861da2 100644 --- a/src/fsfw/osal/linux/Clock.cpp +++ b/src/fsfw/osal/linux/Clock.cpp @@ -42,7 +42,7 @@ ReturnValue_t Clock::setClock(const timeval* time) { return returnvalue::OK; } -ReturnValue_t Clock::getClock(timeval *time) { +ReturnValue_t Clock::getClock(timeval* time) { timespec timeUnix{}; int status = clock_gettime(CLOCK_REALTIME, &timeUnix); if (status != 0) { @@ -53,9 +53,7 @@ ReturnValue_t Clock::getClock(timeval *time) { return returnvalue::OK; } -ReturnValue_t Clock::getClock_timeval(timeval* time) { - return Clock::getClock(time); -} +ReturnValue_t Clock::getClock_timeval(timeval* time) { return Clock::getClock(time); } ReturnValue_t Clock::getClock_usecs(uint64_t* time) { timeval timeVal{}; @@ -68,7 +66,7 @@ ReturnValue_t Clock::getClock_usecs(uint64_t* time) { return returnvalue::OK; } -ReturnValue_t Clock::getClockMonotonic(timeval *time) { +ReturnValue_t Clock::getClockMonotonic(timeval* time) { timespec timeMonotonic{}; int status = clock_gettime(CLOCK_MONOTONIC_RAW, &timeMonotonic); if (status != 0) { diff --git a/src/fsfw/pus/CServiceHealthCommanding.cpp b/src/fsfw/pus/CServiceHealthCommanding.cpp index ebb286cf..da4593ae 100644 --- a/src/fsfw/pus/CServiceHealthCommanding.cpp +++ b/src/fsfw/pus/CServiceHealthCommanding.cpp @@ -68,7 +68,7 @@ ReturnValue_t CServiceHealthCommanding::prepareCommand(CommandMessage *message, ReturnValue_t result = returnvalue::OK; switch (subservice) { case (Subservice::COMMAND_SET_HEALTH): { - if(tcDataLen != sizeof(object_id_t) + sizeof(HasHealthIF::HealthState)) { + if (tcDataLen != sizeof(object_id_t) + sizeof(HasHealthIF::HealthState)) { return CommandingServiceBase::INVALID_TC; } HealthSetCommand healthCommand; diff --git a/src/fsfw/subsystem/SubsystemBase.cpp b/src/fsfw/subsystem/SubsystemBase.cpp index 66858c39..560cb769 100644 --- a/src/fsfw/subsystem/SubsystemBase.cpp +++ b/src/fsfw/subsystem/SubsystemBase.cpp @@ -21,8 +21,23 @@ SubsystemBase::~SubsystemBase() { QueueFactory::instance()->deleteMessageQueue(c ReturnValue_t SubsystemBase::checkStateAgainstTable(HybridIterator tableIter, Submode_t targetSubmode) { + using namespace mode; std::map::iterator childIter; + auto checkSubmode = [&]() { + if (tableIter.value->inheritSubmode()) { + if (childIter->second.submode != targetSubmode) { + return returnvalue::FAILED; + } + } + uint8_t mask; + if (tableIter.value->submodesAllowed(&mask)) { + if ((childIter->second.submode | mask) != mask) { + return returnvalue::FAILED; + } + } + return returnvalue::OK; + }; for (; tableIter.value != NULL; ++tableIter) { object_id_t object = tableIter.value->getObject(); @@ -33,14 +48,9 @@ ReturnValue_t SubsystemBase::checkStateAgainstTable(HybridIteratorsecond.mode != tableIter.value->getMode()) { return returnvalue::FAILED; } - - Submode_t submodeToCheckAgainst = tableIter.value->getSubmode(); - if (tableIter.value->inheritSubmode()) { - submodeToCheckAgainst = targetSubmode; - } - - if (childIter->second.submode != submodeToCheckAgainst) { - return returnvalue::FAILED; + ReturnValue_t result = checkSubmode(); + if (result != returnvalue::OK) { + return result; } } return returnvalue::OK; diff --git a/src/fsfw/subsystem/modes/ModeDefinitions.h b/src/fsfw/subsystem/modes/ModeDefinitions.h index d22bcd95..4b938a68 100644 --- a/src/fsfw/subsystem/modes/ModeDefinitions.h +++ b/src/fsfw/subsystem/modes/ModeDefinitions.h @@ -1,111 +1,102 @@ #ifndef FSFW_SUBSYSTEM_MODES_MODEDEFINITIONS_H_ #define FSFW_SUBSYSTEM_MODES_MODEDEFINITIONS_H_ -#include "../../modes/HasModesIF.h" -#include "../../objectmanager/SystemObjectIF.h" -#include "../../serialize/SerialLinkedListAdapter.h" -#include "../../serialize/SerializeIF.h" +#include "fsfw/modes/HasModesIF.h" +#include "fsfw/objectmanager/SystemObjectIF.h" +#include "fsfw/serialize/SerialLinkedListAdapter.h" +#include "fsfw/serialize/SerializeIF.h" -class ModeListEntry : public SerializeIF, public LinkedElement { +namespace mode { +enum SpecialSubmodeFlags : uint8_t { INHERIT = 1 << 0, ALLOWED_MASK = 1 << 1 }; +} + +class ModeListEntry : public SerialLinkedListAdapter, + public LinkedElement { public: - ModeListEntry() : LinkedElement(this) {} + ModeListEntry() : LinkedElement(this) { setLinks(); } - uint32_t value1 = 0; - uint32_t value2 = 0; - uint8_t value3 = 0; - uint8_t value4 = 0; + SerializeElement value1 = 0; + SerializeElement value2 = 0; + SerializeElement value3 = 0; + SerializeElement value4 = 0; + SerializeElement value5 = 0; - virtual ReturnValue_t serialize(uint8_t** buffer, size_t* size, size_t maxSize, - Endianness streamEndianness) const { - ReturnValue_t result; - - result = SerializeAdapter::serialize(&value1, buffer, size, maxSize, streamEndianness); - - if (result != returnvalue::OK) { - return result; - } - result = SerializeAdapter::serialize(&value2, buffer, size, maxSize, streamEndianness); - - if (result != returnvalue::OK) { - return result; - } - result = SerializeAdapter::serialize(&value3, buffer, size, maxSize, streamEndianness); - - if (result != returnvalue::OK) { - return result; - } - - result = SerializeAdapter::serialize(&value4, buffer, size, maxSize, streamEndianness); - - return result; - } - - virtual size_t getSerializedSize() const { - return sizeof(value1) + sizeof(value2) + sizeof(value3) + sizeof(value4); - } - - virtual ReturnValue_t deSerialize(const uint8_t** buffer, size_t* size, - Endianness streamEndianness) { - ReturnValue_t result; - - result = SerializeAdapter::deSerialize(&value1, buffer, size, streamEndianness); - - if (result != returnvalue::OK) { - return result; - } - result = SerializeAdapter::deSerialize(&value2, buffer, size, streamEndianness); - - if (result != returnvalue::OK) { - return result; - } - result = SerializeAdapter::deSerialize(&value3, buffer, size, streamEndianness); - - if (result != returnvalue::OK) { - return result; - } - result = SerializeAdapter::deSerialize(&value4, buffer, size, streamEndianness); - - return result; + void setLinks() { + setStart(&value1); + value1.setNext(&value2); + value2.setNext(&value3); + value3.setNext(&value4); + value4.setNext(&value5); } // for Sequences - Mode_t getTableId() const { return value1; } + Mode_t getTableId() const { return value1.entry; } - void setTableId(Mode_t tableId) { this->value1 = tableId; } + void setTableId(Mode_t tableId) { this->value1.entry = tableId; } - uint8_t getWaitSeconds() const { return value2; } + uint8_t getWaitSeconds() const { return value2.entry; } - void setWaitSeconds(uint8_t waitSeconds) { this->value2 = waitSeconds; } + void setWaitSeconds(uint8_t waitSeconds) { this->value2.entry = waitSeconds; } - bool checkSuccess() const { return value3 == 1; } + bool checkSuccess() const { return value3.entry == 1; } - void setCheckSuccess(bool checkSuccess) { this->value3 = checkSuccess; } + void setCheckSuccess(bool checkSuccess) { this->value3.entry = checkSuccess; } // for Tables - object_id_t getObject() const { return value1; } + object_id_t getObject() const { return value1.entry; } - void setObject(object_id_t object) { this->value1 = object; } + void setObject(object_id_t object) { this->value1.entry = object; } - Mode_t getMode() const { return value2; } + Mode_t getMode() const { return value2.entry; } - void setMode(Mode_t mode) { this->value2 = mode; } + void setMode(Mode_t mode) { this->value2.entry = mode; } - Submode_t getSubmode() const { return value3; } + Submode_t getSubmode() const { return value3.entry; } - void setSubmode(Submode_t submode) { this->value3 = submode; } + void setSubmode(Submode_t submode) { this->value3.entry = submode; } - bool inheritSubmode() const { return value4 == 1; } - - void setInheritSubmode(bool inherit) { - if (inherit) { - value4 = 1; - } else { - value4 = 0; + bool inheritSubmode() const { + return (value4.entry & mode::SpecialSubmodeFlags::INHERIT) == + mode::SpecialSubmodeFlags::INHERIT; + } + bool submodesAllowed(uint8_t* mask) const { + bool submodesAllowed = (value4.entry & mode::SpecialSubmodeFlags::ALLOWED_MASK) == + mode::SpecialSubmodeFlags::ALLOWED_MASK; + if (submodesAllowed and mask != nullptr) { + *mask = value5.entry; } + return submodesAllowed; } - bool operator==(ModeListEntry other) { - return ((value1 == other.value1) && (value2 == other.value2) && (value3 == other.value3)); + /** + * Enable the inheritance of submodes. This is relevant for both the execution + * of mode tables and for mode checking. + */ + void enableInheritSubmode() { value4.entry |= mode::SpecialSubmodeFlags::INHERIT; } + /** + * Disable the inheritance of submodes. This is relevant for both the execution + * of mode tables and for mode checking. + */ + void disableInheritSubmode() { value4.entry &= ~mode::SpecialSubmodeFlags::INHERIT; } + + /** + * Specialization of @enableSubmodeAllowed which allows all submodes. + */ + void allowAllSubmodes() { enableSubmodeAllowed(0xff); } + + /** + * Enable an allowed submode mask for mode checks. + */ + void enableSubmodeAllowed(uint8_t mask) { + value4.entry |= mode::SpecialSubmodeFlags::ALLOWED_MASK; + value5.entry = mask; + } + /** + * Enforce the equality of submodes for mode checks. This is the default. + */ + void disableSubmodeAllowed() { + value4.entry &= ~mode::SpecialSubmodeFlags::ALLOWED_MASK; + value5.entry = 0; } }; diff --git a/unittests/CMakeLists.txt b/unittests/CMakeLists.txt index ad26e392..950b96b8 100644 --- a/unittests/CMakeLists.txt +++ b/unittests/CMakeLists.txt @@ -13,6 +13,7 @@ add_subdirectory(util) add_subdirectory(container) add_subdirectory(osal) add_subdirectory(pus) +add_subdirectory(subsystem) add_subdirectory(serialize) add_subdirectory(datapoollocal) add_subdirectory(storagemanager) diff --git a/unittests/subsystem/CMakeLists.txt b/unittests/subsystem/CMakeLists.txt new file mode 100644 index 00000000..724ebc05 --- /dev/null +++ b/unittests/subsystem/CMakeLists.txt @@ -0,0 +1 @@ +target_sources(${FSFW_TEST_TGT} PRIVATE testModeDef.cpp) diff --git a/unittests/subsystem/testModeDef.cpp b/unittests/subsystem/testModeDef.cpp new file mode 100644 index 00000000..758b3301 --- /dev/null +++ b/unittests/subsystem/testModeDef.cpp @@ -0,0 +1,49 @@ + +#include +#include + +#include "fsfw/subsystem/modes/ModeDefinitions.h" + +TEST_CASE("Mode Definitions", "[mode]") { + ModeListEntry entry; + + SECTION("Basic") { + entry.setMode(HasModesIF::MODE_OFF); + entry.setSubmode(2); + CHECK(entry.getMode() == HasModesIF::MODE_OFF); + CHECK(entry.getSubmode() == 2); + uint8_t mask; + CHECK(entry.submodesAllowed(&mask) == false); + } + + SECTION("Allowed submode mask") { + entry.allowAllSubmodes(); + uint8_t mask; + CHECK(entry.submodesAllowed(&mask) == true); + CHECK(mask == 0xff); + } + + SECTION("Serialization") { + std::array buf{}; + entry.setObject(0x1f2f3f4f); + entry.setMode(HasModesIF::MODE_ON); + entry.setSubmode(2); + entry.enableInheritSubmode(); + entry.enableSubmodeAllowed(0x1f); + uint8_t* serPtr = buf.data(); + size_t serLen = 0; + REQUIRE(entry.serialize(&serPtr, &serLen, buf.size(), SerializeIF::Endianness::NETWORK) == + returnvalue::OK); + CHECK(buf[0] == 0x1f); + CHECK(buf[1] == 0x2f); + CHECK(buf[2] == 0x3f); + CHECK(buf[3] == 0x4f); + CHECK(buf[4] == 0); + CHECK(buf[5] == 0); + CHECK(buf[6] == 0); + CHECK(buf[7] == HasModesIF::MODE_ON); + CHECK(buf[8] == 2); + CHECK(buf[9] == (mode::SpecialSubmodeFlags::ALLOWED_MASK | mode::SpecialSubmodeFlags::INHERIT)); + CHECK(buf[10] == 0x1f); + } +} From 4c48668125de7a43cb42b8513cada73c2fe3ffbd Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 7 Mar 2023 16:03:28 +0100 Subject: [PATCH 2/4] add copy and assignment ctor for mode definition --- src/fsfw/subsystem/modes/ModeDefinitions.h | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/fsfw/subsystem/modes/ModeDefinitions.h b/src/fsfw/subsystem/modes/ModeDefinitions.h index 4b938a68..b60f01de 100644 --- a/src/fsfw/subsystem/modes/ModeDefinitions.h +++ b/src/fsfw/subsystem/modes/ModeDefinitions.h @@ -13,7 +13,7 @@ enum SpecialSubmodeFlags : uint8_t { INHERIT = 1 << 0, ALLOWED_MASK = 1 << 1 }; class ModeListEntry : public SerialLinkedListAdapter, public LinkedElement { public: - ModeListEntry() : LinkedElement(this) { setLinks(); } + ModeListEntry() : SerialLinkedListAdapter(), LinkedElement(this) { setLinks(); } SerializeElement value1 = 0; SerializeElement value2 = 0; @@ -21,6 +21,25 @@ class ModeListEntry : public SerialLinkedListAdapter, SerializeElement value4 = 0; SerializeElement value5 = 0; + ModeListEntry(const ModeListEntry& other) + : SerialLinkedListAdapter(), LinkedElement(this) { + value1.entry = other.value1.entry; + value2.entry = other.value2.entry; + value3.entry = other.value3.entry; + value4.entry = other.value4.entry; + value5.entry = other.value5.entry; + setLinks(); + } + + ModeListEntry& operator=(const ModeListEntry& other) { + this->value1.entry = other.value1.entry; + this->value2.entry = other.value2.entry; + this->value3.entry = other.value3.entry; + this->value4.entry = other.value4.entry; + this->value5.entry = other.value5.entry; + return *this; + } + void setLinks() { setStart(&value1); value1.setNext(&value2); From af58c414fc40f6151ecb3df3803f9820290e5399 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 7 Mar 2023 16:29:10 +0100 Subject: [PATCH 3/4] bugfix in submode check logic --- src/fsfw/subsystem/SubsystemBase.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/fsfw/subsystem/SubsystemBase.cpp b/src/fsfw/subsystem/SubsystemBase.cpp index 560cb769..a876ae33 100644 --- a/src/fsfw/subsystem/SubsystemBase.cpp +++ b/src/fsfw/subsystem/SubsystemBase.cpp @@ -25,13 +25,18 @@ ReturnValue_t SubsystemBase::checkStateAgainstTable(HybridIterator::iterator childIter; auto checkSubmode = [&]() { + uint8_t mask; + bool submodesAllowedMask = tableIter.value->submodesAllowed(&mask); + uint8_t submodeToCheckAgainst = tableIter.value->getSubmode(); if (tableIter.value->inheritSubmode()) { - if (childIter->second.submode != targetSubmode) { + submodeToCheckAgainst = targetSubmode; + } + if (not submodesAllowedMask) { + if (childIter->second.submode != submodeToCheckAgainst) { return returnvalue::FAILED; } } - uint8_t mask; - if (tableIter.value->submodesAllowed(&mask)) { + if (submodesAllowedMask) { if ((childIter->second.submode | mask) != mask) { return returnvalue::FAILED; } From 070b48ada28e471039dfbc3ac5b964f6cdb8d773 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 7 Mar 2023 17:15:34 +0100 Subject: [PATCH 4/4] review improvements --- src/fsfw/subsystem/SubsystemBase.cpp | 38 +++++++++------------- src/fsfw/subsystem/modes/ModeDefinitions.h | 9 +++-- 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/src/fsfw/subsystem/SubsystemBase.cpp b/src/fsfw/subsystem/SubsystemBase.cpp index a876ae33..eccd447c 100644 --- a/src/fsfw/subsystem/SubsystemBase.cpp +++ b/src/fsfw/subsystem/SubsystemBase.cpp @@ -24,25 +24,6 @@ ReturnValue_t SubsystemBase::checkStateAgainstTable(HybridIterator::iterator childIter; - auto checkSubmode = [&]() { - uint8_t mask; - bool submodesAllowedMask = tableIter.value->submodesAllowed(&mask); - uint8_t submodeToCheckAgainst = tableIter.value->getSubmode(); - if (tableIter.value->inheritSubmode()) { - submodeToCheckAgainst = targetSubmode; - } - if (not submodesAllowedMask) { - if (childIter->second.submode != submodeToCheckAgainst) { - return returnvalue::FAILED; - } - } - if (submodesAllowedMask) { - if ((childIter->second.submode | mask) != mask) { - return returnvalue::FAILED; - } - } - return returnvalue::OK; - }; for (; tableIter.value != NULL; ++tableIter) { object_id_t object = tableIter.value->getObject(); @@ -53,9 +34,22 @@ ReturnValue_t SubsystemBase::checkStateAgainstTable(HybridIteratorsecond.mode != tableIter.value->getMode()) { return returnvalue::FAILED; } - ReturnValue_t result = checkSubmode(); - if (result != returnvalue::OK) { - return result; + + // Check submodes here. + uint8_t mask; + bool submodesAllowedMask = tableIter.value->submodesAllowed(&mask); + uint8_t submodeToCheckAgainst = tableIter.value->getSubmode(); + if (tableIter.value->inheritSubmode()) { + submodeToCheckAgainst = targetSubmode; + } + if (submodesAllowedMask) { + if ((childIter->second.submode | mask) != mask) { + return returnvalue::FAILED; + } + } else { + if (childIter->second.submode != submodeToCheckAgainst) { + return returnvalue::FAILED; + } } } return returnvalue::OK; diff --git a/src/fsfw/subsystem/modes/ModeDefinitions.h b/src/fsfw/subsystem/modes/ModeDefinitions.h index b60f01de..70a66fa6 100644 --- a/src/fsfw/subsystem/modes/ModeDefinitions.h +++ b/src/fsfw/subsystem/modes/ModeDefinitions.h @@ -13,6 +13,8 @@ enum SpecialSubmodeFlags : uint8_t { INHERIT = 1 << 0, ALLOWED_MASK = 1 << 1 }; class ModeListEntry : public SerialLinkedListAdapter, public LinkedElement { public: + static constexpr uint8_t ALL_SUBMODES_ALLOWED_MASK = 0xff; + ModeListEntry() : SerialLinkedListAdapter(), LinkedElement(this) { setLinks(); } SerializeElement value1 = 0; @@ -101,10 +103,13 @@ class ModeListEntry : public SerialLinkedListAdapter, /** * Specialization of @enableSubmodeAllowed which allows all submodes. */ - void allowAllSubmodes() { enableSubmodeAllowed(0xff); } + void allowAllSubmodes() { enableSubmodeAllowed(ALL_SUBMODES_ALLOWED_MASK); } /** - * Enable an allowed submode mask for mode checks. + * Enable an allowed submode mask for mode checks. Any submode which contains bits + * outside of the mask will be declined. + * + * For example, for a mask of 0b11, only the modes 0b00, 0b01 and 0b11 will be accepted. */ void enableSubmodeAllowed(uint8_t mask) { value4.entry |= mode::SpecialSubmodeFlags::ALLOWED_MASK;