diff --git a/CHANGELOG.md b/CHANGELOG.md index 34eeb269..a64730fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## 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 - Move some CMake directives further up top so they are not ignored PR: https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/621 - Small bugfix in STM32 HAL for SPI diff --git a/src/fsfw/container/FixedArrayList.h b/src/fsfw/container/FixedArrayList.h index 26a73921..11882537 100644 --- a/src/fsfw/container/FixedArrayList.h +++ b/src/fsfw/container/FixedArrayList.h @@ -2,6 +2,7 @@ #define FIXEDARRAYLIST_H_ #include +#include #include "ArrayList.h" /** @@ -9,10 +10,8 @@ */ template class FixedArrayList : public ArrayList { -#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::max(), "count_t is not large enough to hold MAX_SIZE"); -#endif private: T data[MAX_SIZE]; diff --git a/src/fsfw/osal/common/TcpTmTcServer.cpp b/src/fsfw/osal/common/TcpTmTcServer.cpp index 2e6910c5..91cb9574 100644 --- a/src/fsfw/osal/common/TcpTmTcServer.cpp +++ b/src/fsfw/osal/common/TcpTmTcServer.cpp @@ -19,6 +19,8 @@ #include #elif defined(PLATFORM_UNIX) #include + +#include #endif const std::string TcpTmTcServer::DEFAULT_SERVER_PORT = tcpip::DEFAULT_SERVER_PORT; @@ -29,7 +31,7 @@ TcpTmTcServer::TcpTmTcServer(object_id_t objectId, object_id_t tmtcTcpBridge, : SystemObject(objectId), tmtcBridgeId(tmtcTcpBridge), receptionMode(receptionMode), - tcpConfig(customTcpServerPort), + tcpConfig(std::move(customTcpServerPort)), receptionBuffer(receptionBufferSize), ringBuffer(ringBufferSize, true) {} @@ -103,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; @@ -138,7 +140,6 @@ ReturnValue_t TcpTmTcServer::performOperation(uint8_t opCode) { closeSocket(connSocket); connSocket = 0; } - return HasReturnvaluesIF::RETURN_OK; } ReturnValue_t TcpTmTcServer::initializeAfterTaskCreation() { @@ -159,7 +160,7 @@ void TcpTmTcServer::handleServerOperation(socket_t& connSocket) { #endif while (true) { - int retval = recv(connSocket, reinterpret_cast(receptionBuffer.data()), + ssize_t retval = recv(connSocket, reinterpret_cast(receptionBuffer.data()), receptionBuffer.capacity(), tcpConfig.tcpFlags); if (retval == 0) { size_t availableReadData = ringBuffer.getAvailableReadData(); @@ -252,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 validPacketIds) { - this->validPacketIds = validPacketIds; +void TcpTmTcServer::setSpacePacketParsingOptions(std::vector 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)) { @@ -283,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(storeAccessor.data()), + ssize_t retval = send(connSocket, reinterpret_cast(storeAccessor.data()), storeAccessor.size(), tcpConfig.tcpTmFlags); if (retval == static_cast(storeAccessor.size())) { // Packet sent, clear FIFO entry @@ -339,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) { diff --git a/src/fsfw/osal/common/TcpTmTcServer.h b/src/fsfw/osal/common/TcpTmTcServer.h index d062a0ce..faf6e0a1 100644 --- a/src/fsfw/osal/common/TcpTmTcServer.h +++ b/src/fsfw/osal/common/TcpTmTcServer.h @@ -17,6 +17,7 @@ #endif #include +#include #include class TcpTmTcBridge; @@ -44,7 +45,7 @@ class TcpTmTcServer : public SystemObject, public TcpIpBase, public ExecutableOb struct TcpConfig { public: - TcpConfig(std::string tcpPort) : tcpPort(tcpPort) {} + explicit TcpConfig(std::string tcpPort) : tcpPort(std::move(tcpPort)) {} /** * Passed to the recv call @@ -84,7 +85,7 @@ class TcpTmTcServer : public SystemObject, public TcpIpBase, public ExecutableOb size_t ringBufferSize = RING_BUFFER_SIZE, std::string customTcpServerPort = DEFAULT_SERVER_PORT, ReceptionModes receptionMode = ReceptionModes::SPACE_PACKETS); - virtual ~TcpTmTcServer(); + ~TcpTmTcServer() override; void enableWiretapping(bool enable); @@ -97,10 +98,10 @@ class TcpTmTcServer : public SystemObject, public TcpIpBase, public ExecutableOb void setSpacePacketParsingOptions(std::vector 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; @@ -115,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; diff --git a/src/fsfw/pus/CService201HealthCommanding.cpp b/src/fsfw/pus/CService201HealthCommanding.cpp index 4d9806f9..f6c49cd3 100644 --- a/src/fsfw/pus/CService201HealthCommanding.cpp +++ b/src/fsfw/pus/CService201HealthCommanding.cpp @@ -13,8 +13,6 @@ CService201HealthCommanding::CService201HealthCommanding(object_id_t objectId, u : CommandingServiceBase(objectId, apid, serviceId, numParallelCommands, commandTimeoutSeconds) { } -CService201HealthCommanding::~CService201HealthCommanding() {} - ReturnValue_t CService201HealthCommanding::isValidSubservice(uint8_t subservice) { switch (subservice) { case (Subservice::COMMAND_SET_HEALTH): @@ -43,8 +41,8 @@ ReturnValue_t CService201HealthCommanding::getMessageQueueAndObject(uint8_t subs } ReturnValue_t CService201HealthCommanding::checkInterfaceAndAcquireMessageQueue( - MessageQueueId_t *messageQueueToSet, object_id_t *objectId) { - HasHealthIF *destination = ObjectManager::instance()->get(*objectId); + MessageQueueId_t *messageQueueToSet, const object_id_t *objectId) { + auto *destination = ObjectManager::instance()->get(*objectId); if (destination == nullptr) { return CommandingServiceBase::INVALID_OBJECT; } @@ -77,6 +75,10 @@ ReturnValue_t CService201HealthCommanding::prepareCommand(CommandMessage *messag HealthMessage::setHealthMessage(message, HealthMessage::HEALTH_ANNOUNCE_ALL); break; } + default: { + // Should never happen, subservice was already checked + result = RETURN_FAILED; + } } return result; } @@ -95,10 +97,9 @@ ReturnValue_t CService201HealthCommanding::handleReply(const CommandMessage *rep } // Not used for now, health state already reported by event -ReturnValue_t CService201HealthCommanding::prepareHealthSetReply(const CommandMessage *reply) { - prepareHealthSetReply(reply); - uint8_t health = static_cast(HealthMessage::getHealth(reply)); - uint8_t oldHealth = static_cast(HealthMessage::getOldHealth(reply)); +[[maybe_unused]] ReturnValue_t CService201HealthCommanding::prepareHealthSetReply(const CommandMessage *reply) { + auto health = static_cast(HealthMessage::getHealth(reply)); + auto oldHealth = static_cast(HealthMessage::getOldHealth(reply)); HealthSetReply healthSetReply(health, oldHealth); return sendTmPacket(Subservice::REPLY_HEALTH_SET, &healthSetReply); } diff --git a/src/fsfw/pus/CService201HealthCommanding.h b/src/fsfw/pus/CService201HealthCommanding.h index 5e52ab39..7ffa06d2 100644 --- a/src/fsfw/pus/CService201HealthCommanding.h +++ b/src/fsfw/pus/CService201HealthCommanding.h @@ -1,7 +1,7 @@ #ifndef FSFW_PUS_CSERVICE201HEALTHCOMMANDING_H_ #define FSFW_PUS_CSERVICE201HEALTHCOMMANDING_H_ -#include "../tmtcservices/CommandingServiceBase.h" +#include "fsfw/tmtcservices/CommandingServiceBase.h" /** * @brief Custom PUS service to set health of all objects @@ -21,7 +21,7 @@ class CService201HealthCommanding : public CommandingServiceBase { public: CService201HealthCommanding(object_id_t objectId, uint16_t apid, uint8_t serviceId, uint8_t numParallelCommands = 4, uint16_t commandTimeoutSeconds = 60); - virtual ~CService201HealthCommanding(); + ~CService201HealthCommanding() override = default; protected: /* CSB abstract function implementations */ @@ -38,12 +38,10 @@ class CService201HealthCommanding : public CommandingServiceBase { bool *isStep) override; private: - ReturnValue_t checkAndAcquireTargetID(object_id_t *objectIdToSet, const uint8_t *tcData, - size_t tcDataLen); - ReturnValue_t checkInterfaceAndAcquireMessageQueue(MessageQueueId_t *MessageQueueToSet, - object_id_t *objectId); + static ReturnValue_t checkInterfaceAndAcquireMessageQueue(MessageQueueId_t *MessageQueueToSet, + const object_id_t *objectId); - ReturnValue_t prepareHealthSetReply(const CommandMessage *reply); + [[maybe_unused]] ReturnValue_t prepareHealthSetReply(const CommandMessage *reply); enum Subservice { //! [EXPORT] : [TC] Set health of target object diff --git a/src/fsfw/tmtcservices/CommandingServiceBase.h b/src/fsfw/tmtcservices/CommandingServiceBase.h index 867fc287..4dcad024 100644 --- a/src/fsfw/tmtcservices/CommandingServiceBase.h +++ b/src/fsfw/tmtcservices/CommandingServiceBase.h @@ -166,7 +166,7 @@ class CommandingServiceBase : public SystemObject, * @param objectId Target object ID * @return * - @c RETURN_OK to generate a verification start message - * - @c EXECUTION_COMPELTE Fire-and-forget command. Generate a completion + * - @c EXECUTION_COMPLETE Fire-and-forget command. Generate a completion * verification message. * - @c Anything else rejects the packets and generates a start failure * verification.