diff --git a/src/fsfw/datapool/PoolDataSetBase.cpp b/src/fsfw/datapool/PoolDataSetBase.cpp index 0df21d2a..2996a496 100644 --- a/src/fsfw/datapool/PoolDataSetBase.cpp +++ b/src/fsfw/datapool/PoolDataSetBase.cpp @@ -82,19 +82,19 @@ uint16_t PoolDataSetBase::getFillCount() const { return fillCount; } ReturnValue_t PoolDataSetBase::readVariable(uint16_t count) { ReturnValue_t result = returnvalue::OK; if (registeredVariables[count] == nullptr) { - /* Configuration error. */ + // Configuration error. return returnvalue::FAILED; } - /* These checks are often performed by the respective variable implementation too, but I guess - a double check does not hurt. */ + // These checks are often performed by the respective variable implementation too, but I guess + // a double check does not hurt. if (registeredVariables[count]->getReadWriteMode() != PoolVariableIF::VAR_WRITE and registeredVariables[count]->getDataPoolId() != PoolVariableIF::NO_PARAMETER) { if (protectEveryReadCommitCall) { result = registeredVariables[count]->read(timeoutTypeForSingleVars, mutexTimeoutForSingleVars); } else { - /* The readWithoutLock function is protected, so we use the attorney here */ + // The readWithoutLock function is protected, so we use the attorney here result = ReadCommitIFAttorney::readWithoutLock(registeredVariables[count]); } diff --git a/src/fsfw/datapool/PoolVariableIF.h b/src/fsfw/datapool/PoolVariableIF.h index a15b93a6..a5a7ae14 100644 --- a/src/fsfw/datapool/PoolVariableIF.h +++ b/src/fsfw/datapool/PoolVariableIF.h @@ -23,6 +23,7 @@ class PoolVariableIF : public SerializeIF, public ReadCommitIF { static constexpr uint8_t INTERFACE_ID = CLASS_ID::POOL_VARIABLE_IF; static constexpr ReturnValue_t INVALID_READ_WRITE_MODE = MAKE_RETURN_CODE(0xA0); static constexpr ReturnValue_t INVALID_POOL_ENTRY = MAKE_RETURN_CODE(0xA1); + static constexpr ReturnValue_t INVALID_SHARED_POOL = MAKE_RETURN_CODE(0xA2); static constexpr bool VALID = 1; static constexpr bool INVALID = 0; diff --git a/src/fsfw/datapoollocal/LocalPoolDataSetBase.h b/src/fsfw/datapoollocal/LocalPoolDataSetBase.h index 9e9b3d85..f58c97f8 100644 --- a/src/fsfw/datapoollocal/LocalPoolDataSetBase.h +++ b/src/fsfw/datapoollocal/LocalPoolDataSetBase.h @@ -42,7 +42,6 @@ class PeriodicHousekeepingHelper; * @ingroup data_pool */ class LocalPoolDataSetBase : public SerializeIF, public PoolDataSetIF { - friend class LocalPoolDataSetAttorney; friend class PeriodicHousekeepingHelper; public: diff --git a/src/fsfw/datapoollocal/LocalPoolObjectBase.cpp b/src/fsfw/datapoollocal/LocalPoolObjectBase.cpp index 837c0278..7b3f437f 100644 --- a/src/fsfw/datapoollocal/LocalPoolObjectBase.cpp +++ b/src/fsfw/datapoollocal/LocalPoolObjectBase.cpp @@ -49,6 +49,7 @@ LocalPoolObjectBase::LocalPoolObjectBase(object_id_t poolOwner, lp_id_t poolId, #endif return; } + sharedPool = hkOwner->getOptionalSharedPool(); if (dataSet != nullptr) { dataSet->registerVariable(this); diff --git a/src/fsfw/datapoollocal/LocalPoolVariable.tpp b/src/fsfw/datapoollocal/LocalPoolVariable.tpp index 3b518e6c..1776205b 100644 --- a/src/fsfw/datapoollocal/LocalPoolVariable.tpp +++ b/src/fsfw/datapoollocal/LocalPoolVariable.tpp @@ -39,6 +39,9 @@ inline ReturnValue_t LocalPoolVariable::read(MutexIF::TimeoutType timeoutType template inline ReturnValue_t LocalPoolVariable::readWithoutLock() { + if (sharedPool == nullptr) { + return PoolVariableIF::INVALID_SHARED_POOL; + } if (readWriteMode == pool_rwm_t::VAR_WRITE) { return PoolVariableIF::INVALID_READ_WRITE_MODE; } diff --git a/src/fsfw/datapoollocal/PeriodicHkGenerationHelper.cpp b/src/fsfw/datapoollocal/PeriodicHkGenerationHelper.cpp index 40dc2f98..f3cf5670 100644 --- a/src/fsfw/datapoollocal/PeriodicHkGenerationHelper.cpp +++ b/src/fsfw/datapoollocal/PeriodicHkGenerationHelper.cpp @@ -143,7 +143,7 @@ ReturnValue_t PeriodicHkGenerationHelper::generateHousekeepingPacket(sid_t sid, if (!optSetSpec.has_value()) { return DATASET_NOT_FOUND; } - auto setSpec = optSetSpec.value(); + auto& setSpec = optSetSpec.value().get(); uint8_t* dataPtr = nullptr; const size_t maxSize = setSpec.size; ReturnValue_t result = ipcStore->getFreeElement(&storeId, maxSize, &dataPtr); @@ -185,9 +185,6 @@ ReturnValue_t PeriodicHkGenerationHelper::generateHousekeepingPacket(sid_t sid, return hkQueue->sendMessage(destination, &hkMessage); } -ReturnValue_t PeriodicHkGenerationHelper::serializeHkPacketIntoStore( - HousekeepingPacketDownlink& hkPacket, store_address_t& storeId, size_t* serializedSize) {} - void PeriodicHkGenerationHelper::performPeriodicHkGeneration(periodicHk::SetSpecification& setSpec, timeval& now) { if (not setSpec.periodicCollectionEnabled) { @@ -225,12 +222,12 @@ ReturnValue_t PeriodicHkGenerationHelper::togglePeriodicGeneration(sid_t sid, bo DATASET_NOT_FOUND); return DATASET_NOT_FOUND; } - optSetSpec.value().periodicCollectionEnabled = enable; + optSetSpec.value().get().periodicCollectionEnabled = enable; return returnvalue::OK; } -std::optional PeriodicHkGenerationHelper::getSetSpecification( - sid_t structureId) { +std::optional> +PeriodicHkGenerationHelper::getSetSpecification(sid_t structureId) { for (auto& receiver : setList) { if (receiver.dataId.sid == structureId) { return receiver; @@ -263,7 +260,7 @@ ReturnValue_t PeriodicHkGenerationHelper::generateSetStructurePacket(sid_t sid) DATASET_NOT_FOUND); return DATASET_NOT_FOUND; } - auto setSpec = *optSetSpec; + auto& setSpec = optSetSpec.value().get(); uint8_t* storePtr = nullptr; store_address_t storeId; @@ -314,7 +311,7 @@ ReturnValue_t PeriodicHkGenerationHelper::enablePeriodicPacket( DATASET_NOT_FOUND); return DATASET_NOT_FOUND; } - auto setSpec = optSetSpec.value(); + auto& setSpec = optSetSpec.value().get(); setSpec.periodicCollectionEnabled = true; if (frequencyMs) { setSpec.collectionFrequency = frequencyMs.value(); @@ -330,7 +327,7 @@ ReturnValue_t PeriodicHkGenerationHelper::disablePeriodicPacket(sid_t structureI DATASET_NOT_FOUND); return DATASET_NOT_FOUND; } - auto setSpec = optSetSpec.value(); + auto& setSpec = optSetSpec.value().get(); setSpec.periodicCollectionEnabled = false; return returnvalue::OK; } diff --git a/src/fsfw/datapoollocal/PeriodicHkGenerationHelper.h b/src/fsfw/datapoollocal/PeriodicHkGenerationHelper.h index a1afacfc..9fe65c5d 100644 --- a/src/fsfw/datapoollocal/PeriodicHkGenerationHelper.h +++ b/src/fsfw/datapoollocal/PeriodicHkGenerationHelper.h @@ -213,7 +213,8 @@ class PeriodicHkGenerationHelper : public PeriodicHkGenerationProviderIF { ReturnValue_t setCollectionInterval(sid_t structureId, dur_millis_t newCollectionIntervalMs); protected: - std::optional getSetSpecification(sid_t structureId); + std::optional> getSetSpecification( + sid_t structureId); ReturnValue_t subscribeForPeriodicPacket(subdp::ParamsBase& params); ReturnValue_t subscribeForUpdatePacket(subdp::ParamsBase& params); diff --git a/src/fsfw/datapoollocal/SharedPool.h b/src/fsfw/datapoollocal/SharedPool.h index 6045c641..695e87d9 100644 --- a/src/fsfw/datapoollocal/SharedPool.h +++ b/src/fsfw/datapoollocal/SharedPool.h @@ -35,7 +35,7 @@ class SharedPool { object_id_t ownerId; // Core data structure for the actual pool data - DataPool localPoolMap; + DataPool localPoolMap{}; // Every housekeeping data manager has a mutex to protect access // to it's data pool. MutexIF* mutex = nullptr; diff --git a/unittests/datapoollocal/testDataSet.cpp b/unittests/datapoollocal/testDataSet.cpp index 2692b48b..061741a6 100644 --- a/unittests/datapoollocal/testDataSet.cpp +++ b/unittests/datapoollocal/testDataSet.cpp @@ -30,12 +30,12 @@ TEST_CASE("DataSetTest", "[DataSetTest]") { CHECK(localSet.getCreatorObjectId() == objects::TEST_LOCAL_POOL_OWNER_BASE); size_t maxSize = localSet.getLocalPoolIdsSerializedSize(true); uint8_t localPoolIdBuff[maxSize]; - /* Skip size field */ + // Skip size field auto* lpIds = reinterpret_cast(localPoolIdBuff + 1); size_t serSize = 0; auto* localPoolIdBuffPtr = reinterpret_cast(localPoolIdBuff); - /* Test local pool ID serialization */ + // Test local pool ID serialization CHECK(localSet.serializeLocalPoolIds(&localPoolIdBuffPtr, &serSize, maxSize, SerializeIF::Endianness::MACHINE) == returnvalue::OK); CHECK(serSize == maxSize); @@ -43,7 +43,7 @@ TEST_CASE("DataSetTest", "[DataSetTest]") { CHECK(lpIds[0] == localSet.localPoolVarUint8.getDataPoolId()); CHECK(lpIds[1] == localSet.localPoolVarFloat.getDataPoolId()); CHECK(lpIds[2] == localSet.localPoolUint16Vec.getDataPoolId()); - /* Now serialize without fill count */ + // Now serialize without fill count lpIds = reinterpret_cast(localPoolIdBuff); localPoolIdBuffPtr = localPoolIdBuff; serSize = 0; @@ -55,29 +55,24 @@ TEST_CASE("DataSetTest", "[DataSetTest]") { CHECK(lpIds[2] == localSet.localPoolUint16Vec.getDataPoolId()); { - /* Test read operation. Values should be all zeros */ + // Test read operation. Values should be all zeros PoolReadGuard readHelper(&localSet); REQUIRE(readHelper.getReadResult() == returnvalue::OK); - // CHECK(not localSet.isValid()); CHECK(localSet.localPoolVarUint8.value == 0); - // CHECK(not localSet.localPoolVarUint8.isValid()); CHECK(localSet.localPoolVarFloat.value == Catch::Approx(0.0)); - // CHECK(not localSet.localPoolVarUint8.isValid()); CHECK(localSet.localPoolUint16Vec.value[0] == 0); CHECK(localSet.localPoolUint16Vec.value[1] == 0); CHECK(localSet.localPoolUint16Vec.value[2] == 0); - // CHECK(not localSet.localPoolVarUint8.isValid()); - /* Now set new values, commit should be done by read helper automatically */ + // Now set new values, commit should be done by read helper automatically localSet.localPoolVarUint8 = 232; localSet.localPoolVarFloat = -2324.322; localSet.localPoolUint16Vec.value[0] = 232; localSet.localPoolUint16Vec.value[1] = 23923; localSet.localPoolUint16Vec.value[2] = 1; - // localSet.setValidity(true, true); } - /* Zero out some values for next test */ + // Zero out some values for next test localSet.localPoolVarUint8 = 0; localSet.localPoolVarFloat = 0; @@ -87,25 +82,21 @@ TEST_CASE("DataSetTest", "[DataSetTest]") { CHECK(localSet.localPoolVarFloat.getReadWriteMode() == pool_rwm_t::VAR_READ); { - /* Now we read again and check whether our zeroed values were overwritten with - the values in the pool */ + // Now we read again and check whether our zeroed values were overwritten with + // the values in the pool PoolReadGuard readHelper(&localSet); REQUIRE(readHelper.getReadResult() == returnvalue::OK); - // CHECK(localSet.isValid()); CHECK(localSet.localPoolVarUint8.value == 232); - // CHECK(localSet.localPoolVarUint8.isValid()); CHECK(localSet.localPoolVarFloat.value == Catch::Approx(-2324.322)); - // CHECK(localSet.localPoolVarFloat.isValid()); CHECK(localSet.localPoolUint16Vec.value[0] == 232); CHECK(localSet.localPoolUint16Vec.value[1] == 23923); CHECK(localSet.localPoolUint16Vec.value[2] == 1); - // CHECK(localSet.localPoolUint16Vec.isValid()); // Now we serialize these values into a buffer without the validity buffer maxSize = localSet.getSerializedSize(); CHECK(maxSize == sizeof(uint8_t) + sizeof(uint16_t) * 3 + sizeof(float)); serSize = 0; - /* Already reserve additional space for validity buffer, will be needed later */ + // Already reserve additional space for validity buffer, will be needed later uint8_t buffer[maxSize + 1]; uint8_t* buffPtr = buffer; CHECK(localSet.serialize(&buffPtr, &serSize, maxSize, SerializeIF::Endianness::MACHINE) == @@ -123,79 +114,18 @@ TEST_CASE("DataSetTest", "[DataSetTest]") { CHECK(rawUint16Vec[2] == 1); size_t sizeToDeserialize = maxSize; - /* Now we zeros out the raw entries and deserialize back into the dataset */ + // Now we zeros out the raw entries and deserialize back into the dataset std::memset(buffer, 0, sizeof(buffer)); const uint8_t* constBuffPtr = buffer; CHECK(localSet.deSerialize(&constBuffPtr, &sizeToDeserialize, SerializeIF::Endianness::MACHINE) == returnvalue::OK); - /* Check whether deserialization was successfull */ + // Check whether deserialization was successfull CHECK(localSet.localPoolVarUint8.value == 0); CHECK(localSet.localPoolVarFloat.value == Catch::Approx(0.0)); CHECK(localSet.localPoolVarUint8.value == 0); CHECK(localSet.localPoolUint16Vec.value[0] == 0); CHECK(localSet.localPoolUint16Vec.value[1] == 0); CHECK(localSet.localPoolUint16Vec.value[2] == 0); - /* Validity should be unchanged */ - // CHECK(localSet.localPoolVarUint8.isValid()); - // CHECK(localSet.localPoolVarFloat.isValid()); - // CHECK(localSet.localPoolUint16Vec.isValid()); - - /* Now we do the same process but with the validity buffer */ - localSet.localPoolVarUint8 = 232; - localSet.localPoolVarFloat = -2324.322; - localSet.localPoolUint16Vec.value[0] = 232; - localSet.localPoolUint16Vec.value[1] = 23923; - localSet.localPoolUint16Vec.value[2] = 1; - // localSet.localPoolVarUint8.setValid(true); - // localSet.localPoolVarFloat.setValid(false); - // localSet.localPoolUint16Vec.setValid(true); - // localSet.setValidityBufferGeneration(true); - maxSize = localSet.getSerializedSize(); - CHECK(maxSize == sizeof(uint8_t) + sizeof(uint16_t) * 3 + sizeof(float) + 1); - serSize = 0; - buffPtr = buffer; - CHECK(localSet.serialize(&buffPtr, &serSize, maxSize, SerializeIF::Endianness::MACHINE) == - returnvalue::OK); - CHECK(rawUint8 == 232); - std::memcpy(&rawFloat, buffer + sizeof(uint8_t), sizeof(float)); - CHECK(rawFloat == Catch::Approx(-2324.322)); - - std::memcpy(&rawUint16Vec, buffer + sizeof(uint8_t) + sizeof(float), 3 * sizeof(uint16_t)); - CHECK(rawUint16Vec[0] == 232); - CHECK(rawUint16Vec[1] == 23923); - CHECK(rawUint16Vec[2] == 1); - /* We can do it like this because the buffer only has one byte for - less than 8 variables */ - uint8_t* validityByte = buffer + sizeof(buffer) - 1; - bool bitSet = false; - bitutil::get(validityByte, 0, bitSet); - - CHECK(bitSet == true); - bitutil::get(validityByte, 1, bitSet); - CHECK(bitSet == false); - bitutil::get(validityByte, 2, bitSet); - CHECK(bitSet == true); - - /* Now we manipulate the validity buffer for the deserialization */ - bitutil::clear(validityByte, 0); - bitutil::set(validityByte, 1); - bitutil::clear(validityByte, 2); - /* Zero out everything except validity buffer */ - std::memset(buffer, 0, sizeof(buffer) - 1); - sizeToDeserialize = maxSize; - constBuffPtr = buffer; - CHECK(localSet.deSerialize(&constBuffPtr, &sizeToDeserialize, - SerializeIF::Endianness::MACHINE) == returnvalue::OK); - /* Check whether deserialization was successfull */ - CHECK(localSet.localPoolVarUint8.value == 0); - CHECK(localSet.localPoolVarFloat.value == Catch::Approx(0.0)); - CHECK(localSet.localPoolVarUint8.value == 0); - CHECK(localSet.localPoolUint16Vec.value[0] == 0); - CHECK(localSet.localPoolUint16Vec.value[1] == 0); - CHECK(localSet.localPoolUint16Vec.value[2] == 0); - // CHECK(not localSet.localPoolVarUint8.isValid()); - // CHECK(localSet.localPoolVarFloat.isValid()); - // CHECK(not localSet.localPoolUint16Vec.isValid()); } /* Common fault test cases */ @@ -207,53 +137,6 @@ TEST_CASE("DataSetTest", "[DataSetTest]") { static_cast(DataSetIF::POOL_VAR_NULL)); } - SECTION("MorePoolVariables") { - LocalDataSet set(poolOwner.sharedPool, 2, 10); - - /* Register same variables again to get more than 8 registered variables */ - for (uint8_t idx = 0; idx < 8; idx++) { - REQUIRE(set.registerVariable(&localSet.localPoolVarUint8) == returnvalue::OK); - } - REQUIRE(set.registerVariable(&localSet.localPoolVarUint8) == returnvalue::OK); - REQUIRE(set.registerVariable(&localSet.localPoolUint16Vec) == returnvalue::OK); - - // set.setValidityBufferGeneration(true); - { - PoolReadGuard readHelper(&localSet); - localSet.localPoolVarUint8.value = 42; - // localSet.localPoolVarUint8.setValid(true); - // localSet.localPoolUint16Vec.setValid(false); - } - - size_t maxSize = set.getSerializedSize(); - CHECK(maxSize == 9 + sizeof(uint16_t) * 3 + 2); - size_t serSize = 0; - /* Already reserve additional space for validity buffer, will be needed later */ - uint8_t buffer[maxSize + 1]; - uint8_t* buffPtr = buffer; - CHECK(set.serialize(&buffPtr, &serSize, maxSize, SerializeIF::Endianness::MACHINE) == OK); - std::array validityBuffer{}; - std::memcpy(validityBuffer.data(), buffer + 9 + sizeof(uint16_t) * 3, 2); - /* The first 9 variables should be valid */ - CHECK(validityBuffer[0] == 0xff); - bool bitSet = false; - bitutil::get(validityBuffer.data() + 1, 0, bitSet); - CHECK(bitSet == true); - bitutil::get(validityBuffer.data() + 1, 1, bitSet); - CHECK(bitSet == false); - - /* Now we invert the validity */ - validityBuffer[0] = 0; - validityBuffer[1] = 0b0100'0000; - std::memcpy(buffer + 9 + sizeof(uint16_t) * 3, validityBuffer.data(), 2); - const uint8_t* constBuffPtr = buffer; - size_t sizeToDeSerialize = serSize; - CHECK(set.deSerialize(&constBuffPtr, &sizeToDeSerialize, SerializeIF::Endianness::MACHINE) == - returnvalue::OK); - // CHECK(localSet.localPoolVarUint8.isValid() == false); - // CHECK(localSet.localPoolUint16Vec.isValid() == true); - } - SECTION("SharedDataSet") { object_id_t sharedSetId = objects::SHARED_SET_ID; SharedLocalDataSet sharedSet(sharedSetId, poolOwner.sharedPool, lpool::testSetId, 5); @@ -266,15 +149,13 @@ TEST_CASE("DataSetTest", "[DataSetTest]") { CHECK(sharedSet.unlockDataset() == returnvalue::OK); { - // PoolReadGuard rg(&sharedSet); - // CHECK(rg.getReadResult() == returnvalue::OK); + PoolReadGuard rg(&sharedSet); + CHECK(rg.getReadResult() == returnvalue::OK); localSet.localPoolVarUint8.value = 5; localSet.localPoolUint16Vec.value[0] = 1; localSet.localPoolUint16Vec.value[1] = 2; localSet.localPoolUint16Vec.value[2] = 3; CHECK(sharedSet.commit() == returnvalue::OK); } - - // sharedSet.setReadCommitProtectionBehaviour(true); } }