From e29226c9bba6587460d5db87e3ad3b3d7eab2357 Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Fri, 10 Jul 2020 14:28:36 +0200 Subject: [PATCH 1/6] srv8 improved --- pus/Service8FunctionManagement.cpp | 9 +++++- pus/servicepackets/Service8Packets.h | 41 ++++++++++++++-------------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/pus/Service8FunctionManagement.cpp b/pus/Service8FunctionManagement.cpp index 812398b9..50102b04 100644 --- a/pus/Service8FunctionManagement.cpp +++ b/pus/Service8FunctionManagement.cpp @@ -59,8 +59,15 @@ ReturnValue_t Service8FunctionManagement::prepareCommand( ReturnValue_t Service8FunctionManagement::prepareDirectCommand( CommandMessage *message, const uint8_t *tcData, size_t tcDataLen) { + if(tcDataLen < sizeof(object_id_t) + sizeof(ActionId_t)) { + sif::debug << "Service8FunctionManagement::prepareDirectCommand:" + << " TC size smaller thant minimum size of direct command." + << std::endl; + return CommandingServiceBase::INVALID_TC; + } + // Create direct command instance by extracting data from Telecommand - DirectCommand command(tcData,tcDataLen); + DirectCommand command(tcData, tcDataLen); // store additional parameters into the IPC Store store_address_t parameterAddress; diff --git a/pus/servicepackets/Service8Packets.h b/pus/servicepackets/Service8Packets.h index 5ef91280..86681c14 100644 --- a/pus/servicepackets/Service8Packets.h +++ b/pus/servicepackets/Service8Packets.h @@ -8,10 +8,7 @@ * 3. Return Code * 4. Optional step number for step replies * - * Data reply (subservice 132) consists of - * 1. Target Object ID - * 2. Action ID - * 3. Data + * * \date 01.07.2019 * \author R. Mueller @@ -29,23 +26,21 @@ /** - * \brief Subservice 128 - * \ingroup spacepackets + * @brief Subservice 128 + * @ingroup spacepackets */ class DirectCommand: public SerialLinkedListAdapter { //!< [EXPORT] : [SUBSERVICE] 128 public: - //typedef uint16_t typeOfMaxData; - //static const typeOfMaxData MAX_DATA = 256; - DirectCommand(const uint8_t* dataBuffer_, uint32_t size_) { - size_t size = sizeof(objectId); - SerializeAdapter::deSerialize(&objectId,&dataBuffer_,&size, + + DirectCommand(const uint8_t* tcData, size_t size) { + SerializeAdapter::deSerialize(&objectId, &tcData, &size, SerializeIF::Endianness::BIG); - size = sizeof(actionId); - SerializeAdapter::deSerialize(&actionId,&dataBuffer_,&size, + SerializeAdapter::deSerialize(&actionId, &tcData, &size, SerializeIF::Endianness::BIG); - parameterBuffer = dataBuffer_; - parametersSize = size_ - sizeof(objectId) - sizeof(actionId); + parameterBuffer = tcData; + parametersSize = size; } + ActionId_t getActionId() const { return actionId; } @@ -73,8 +68,12 @@ private: /** - * \brief Subservice 130 - * \ingroup spacepackets + * @brief Subservice 130 + * Data reply (subservice 130) consists of + * 1. Target Object ID + * 2. Action ID + * 3. Data + * @ingroup spacepackets */ class DataReply: public SerialLinkedListAdapter { //!< [EXPORT] : [SUBSERVICE] 130 public: @@ -100,8 +99,10 @@ private: /** - * \brief Subservice 132 - * \ingroup spacepackets + * @brief Subservice 132 + * @details + * Not used yet. Telecommand Verification takes care of this. + * @ingroup spacepackets */ class DirectReply: public SerialLinkedListAdapter { //!< [EXPORT] : [SUBSERVICE] 132 public: @@ -124,7 +125,7 @@ private: returnCode.setNext(&step); } } - bool isDataReply; //!< [EXPORT] : [IGNORE] + bool isStep; //!< [EXPORT] : [IGNORE] SerializeElement objectId; //!< [EXPORT] : [IGNORE] SerializeElement actionId; //!< [EXPORT] : [IGNORE] From 36a7f2f9ee86b00370bee48721f470d9392bf93c Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Fri, 10 Jul 2020 14:32:25 +0200 Subject: [PATCH 2/6] removed file header comment --- pus/servicepackets/Service8Packets.h | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/pus/servicepackets/Service8Packets.h b/pus/servicepackets/Service8Packets.h index 86681c14..8ea0d108 100644 --- a/pus/servicepackets/Service8Packets.h +++ b/pus/servicepackets/Service8Packets.h @@ -1,19 +1,3 @@ -/** - * \file Service8Packets.h - * - * \brief Structure of a Direct Command. - * Normal reply (subservice 130) consists of - * 1. Target object ID - * 2. Action ID (taget device has specified functions with action IDs) - * 3. Return Code - * 4. Optional step number for step replies - * - - * - * \date 01.07.2019 - * \author R. Mueller - */ - #ifndef FRAMEWORK_PUS_SERVICEPACKETS_SERVICE8PACKETS_H_ #define FRAMEWORK_PUS_SERVICEPACKETS_SERVICE8PACKETS_H_ From 31450362106ebec643d99dd60d963e822516b0a5 Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Fri, 10 Jul 2020 19:34:18 +0200 Subject: [PATCH 3/6] getter functions for task handle --- osal/FreeRTOS/FixedTimeslotTask.cpp | 3 +++ osal/FreeRTOS/FixedTimeslotTask.h | 2 ++ osal/FreeRTOS/PeriodicTask.cpp | 4 ++++ osal/FreeRTOS/PeriodicTask.h | 2 ++ osal/FreeRTOS/TaskManagement.cpp | 5 +++-- osal/FreeRTOS/TaskManagement.h | 3 ++- 6 files changed, 16 insertions(+), 3 deletions(-) diff --git a/osal/FreeRTOS/FixedTimeslotTask.cpp b/osal/FreeRTOS/FixedTimeslotTask.cpp index 57c927f9..3bd2aafb 100644 --- a/osal/FreeRTOS/FixedTimeslotTask.cpp +++ b/osal/FreeRTOS/FixedTimeslotTask.cpp @@ -157,3 +157,6 @@ ReturnValue_t FixedTimeslotTask::sleepFor(uint32_t ms) { return HasReturnvaluesIF::RETURN_OK; } +TaskHandle_t FixedTimeslotTask::getTaskHandle() const { + return handle; +} diff --git a/osal/FreeRTOS/FixedTimeslotTask.h b/osal/FreeRTOS/FixedTimeslotTask.h index 27d08190..985b4f90 100644 --- a/osal/FreeRTOS/FixedTimeslotTask.h +++ b/osal/FreeRTOS/FixedTimeslotTask.h @@ -57,6 +57,8 @@ public: ReturnValue_t sleepFor(uint32_t ms) override; + TaskHandle_t getTaskHandle() const; + protected: bool started; TaskHandle_t handle; diff --git a/osal/FreeRTOS/PeriodicTask.cpp b/osal/FreeRTOS/PeriodicTask.cpp index 2ebfebf4..6d013a5b 100644 --- a/osal/FreeRTOS/PeriodicTask.cpp +++ b/osal/FreeRTOS/PeriodicTask.cpp @@ -126,6 +126,10 @@ void PeriodicTask::checkMissedDeadline(const TickType_t xLastWakeTime, } } +TaskHandle_t PeriodicTask::getTaskHandle() const { + return handle; +} + void PeriodicTask::handleMissedDeadline() { #ifdef DEBUG sif::warning << "PeriodicTask: " << pcTaskGetName(NULL) << diff --git a/osal/FreeRTOS/PeriodicTask.h b/osal/FreeRTOS/PeriodicTask.h index 09aa6bc7..679686f6 100644 --- a/osal/FreeRTOS/PeriodicTask.h +++ b/osal/FreeRTOS/PeriodicTask.h @@ -69,6 +69,8 @@ public: uint32_t getPeriodMs() const override; ReturnValue_t sleepFor(uint32_t ms) override; + + TaskHandle_t getTaskHandle() const; protected: bool started; TaskHandle_t handle; diff --git a/osal/FreeRTOS/TaskManagement.cpp b/osal/FreeRTOS/TaskManagement.cpp index 7871ab2e..9a1e4447 100644 --- a/osal/FreeRTOS/TaskManagement.cpp +++ b/osal/FreeRTOS/TaskManagement.cpp @@ -18,6 +18,7 @@ TaskHandle_t TaskManagement::getCurrentTaskHandle() { return xTaskGetCurrentTaskHandle(); } -configSTACK_DEPTH_TYPE TaskManagement::getTaskStackHighWatermark() { - return uxTaskGetStackHighWaterMark(TaskManagement::getCurrentTaskHandle()); +configSTACK_DEPTH_TYPE TaskManagement::getTaskStackHighWatermark( + TaskHandle_t task) { + return uxTaskGetStackHighWaterMark(task); } diff --git a/osal/FreeRTOS/TaskManagement.h b/osal/FreeRTOS/TaskManagement.h index 39c24377..cbde510c 100644 --- a/osal/FreeRTOS/TaskManagement.h +++ b/osal/FreeRTOS/TaskManagement.h @@ -57,7 +57,8 @@ public: * @return Smallest value of stack remaining since the task was started in * words. */ - static configSTACK_DEPTH_TYPE getTaskStackHighWatermark(); + static configSTACK_DEPTH_TYPE getTaskStackHighWatermark( + TaskHandle_t task = nullptr); }; #endif /* FRAMEWORK_OSAL_FREERTOS_TASKMANAGEMENT_H_ */ From 444ee80f356a8e6b8e639c1cb847da320a21713b Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Fri, 10 Jul 2020 20:31:10 +0200 Subject: [PATCH 4/6] removed unnecessary case and added more size checks --- pus/Service2DeviceAccess.cpp | 6 ++---- tmtcservices/PusParser.cpp | 29 ++++++++++++++--------------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/pus/Service2DeviceAccess.cpp b/pus/Service2DeviceAccess.cpp index 1d5f21eb..84d6693d 100644 --- a/pus/Service2DeviceAccess.cpp +++ b/pus/Service2DeviceAccess.cpp @@ -60,13 +60,11 @@ ReturnValue_t Service2DeviceAccess::prepareCommand(CommandMessage* message, uint32_t* state, object_id_t objectId) { switch(static_cast(subservice)){ case Subservice::RAW_COMMANDING: { - return prepareRawCommand(dynamic_cast(message), - tcData, tcDataLen); + return prepareRawCommand(message, tcData, tcDataLen); } break; case Subservice::TOGGLE_WIRETAPPING: { - return prepareWiretappingCommand(dynamic_cast(message), - tcData, tcDataLen); + return prepareWiretappingCommand(message, tcData, tcDataLen); } break; default: diff --git a/tmtcservices/PusParser.cpp b/tmtcservices/PusParser.cpp index 4198e36e..3dd34a84 100644 --- a/tmtcservices/PusParser.cpp +++ b/tmtcservices/PusParser.cpp @@ -7,9 +7,8 @@ PusParser::PusParser(uint16_t maxExpectedPusPackets, ReturnValue_t PusParser::parsePusPackets(const uint8_t *frame, size_t frameSize) { - if(frame == nullptr) { - sif::error << "PusParser::parsePusPackets: Frame pointers in invalid!" - << std::endl; + if(frame == nullptr or frameSize < 5) { + sif::error << "PusParser::parsePusPackets: Frame invalid!" << std::endl; return HasReturnvaluesIF::RETURN_FAILED; } @@ -25,15 +24,13 @@ ReturnValue_t PusParser::parsePusPackets(const uint8_t *frame, } size_t packetSize = lengthField + 7; + // sif::debug << frameSize << std::endl; // Size of a pus packet is the value in the packet length field plus 7. - if(packetSize > frameSize) - { - if(storeSplitPackets) - { + if(packetSize > frameSize) { + if(storeSplitPackets) { indexSizePairFIFO.insert(indexSizePair(0, frameSize)); } - else - { + else { sif::debug << "TcSerialPollingTask::readNextPacket: Next packet " "larger than remaining frame," << std::endl; sif::debug << "Throwing away packet. Detected packet size: " @@ -41,8 +38,7 @@ ReturnValue_t PusParser::parsePusPackets(const uint8_t *frame, } return SPLIT_PACKET; } - else - { + else { indexSizePairFIFO.insert(indexSizePair(0, packetSize)); if(packetSize == frameSize) { return HasReturnvaluesIF::RETURN_OK; @@ -77,6 +73,11 @@ PusParser::indexSizePair PusParser::getNextFifoPair() { ReturnValue_t PusParser::readNextPacket(const uint8_t *frame, size_t frameSize, size_t& currentIndex) { // sif::debug << startIndex << std::endl; + if(currentIndex + 5 > frameSize) { + currentIndex = frameSize; + return HasReturnvaluesIF::RETURN_OK; + } + uint16_t lengthField = frame[currentIndex + 4] << 8 | frame[currentIndex + 5]; if(lengthField == 0) { @@ -88,12 +89,10 @@ ReturnValue_t PusParser::readNextPacket(const uint8_t *frame, size_t remainingSize = frameSize - currentIndex; if(nextPacketSize > remainingSize) { - if(storeSplitPackets) - { + if(storeSplitPackets) { indexSizePairFIFO.insert(indexSizePair(currentIndex, remainingSize)); } - else - { + else { sif::debug << "TcSerialPollingTask::readNextPacket: Next packet " "larger than remaining frame," << std::endl; sif::debug << "Throwing away packet. Detected packet size: " From 6a6395313f8dce6ec9693acddb6b93d3cda9549a Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Sat, 11 Jul 2020 01:06:01 +0200 Subject: [PATCH 5/6] added copy ctor and assignment for FIFObase --- container/DynamicFIFO.h | 2 +- container/FIFOBase.h | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/container/DynamicFIFO.h b/container/DynamicFIFO.h index 3a5242c7..fca4c8e8 100644 --- a/container/DynamicFIFO.h +++ b/container/DynamicFIFO.h @@ -18,7 +18,7 @@ template class DynamicFIFO: public FIFOBase { public: DynamicFIFO(size_t maxCapacity): FIFOBase(values.data(), maxCapacity), - values(maxCapacity) {}; + values(maxCapacity) {}; private: std::vector values; diff --git a/container/FIFOBase.h b/container/FIFOBase.h index 8bdb333f..082c596a 100644 --- a/container/FIFOBase.h +++ b/container/FIFOBase.h @@ -3,6 +3,7 @@ #include #include +#include template class FIFOBase { @@ -43,6 +44,23 @@ public: bool full(); size_t size(); + FIFOBase(const FIFOBase& other): readIndex(other.readIndex), + writeIndex(other.writeIndex), currentSize(other.currentSize), + maxCapacity(other.maxCapacity) { + std::memcpy(values, other.values, sizeof(T) * currentSize); + } + + FIFOBase& operator=(const FIFOBase& other) { + if(&other == this) + return *this; + maxCapacity = other.maxCapacity; + readIndex = other.readIndex; + writeIndex = other.writeIndex; + currentSize = other.currentSize; + std::memcpy(values, other.values, sizeof(T) * currentSize); + return *this; + } + size_t getMaxCapacity() const; private: From 35d4267b400a950847447d2adc24e53096e4ef86 Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Sat, 11 Jul 2020 02:36:04 +0200 Subject: [PATCH 6/6] dynamic fifo bug fixed --- container/DynamicFIFO.h | 21 ++++++++++++++++++--- container/FIFO.h | 12 ++++++++++-- container/FIFOBase.h | 22 ++++------------------ container/FIFOBase.tpp | 8 +++++++- tmtcservices/PusParser.h | 9 +++++---- 5 files changed, 44 insertions(+), 28 deletions(-) diff --git a/container/DynamicFIFO.h b/container/DynamicFIFO.h index fca4c8e8..59adfb3a 100644 --- a/container/DynamicFIFO.h +++ b/container/DynamicFIFO.h @@ -17,11 +17,26 @@ template class DynamicFIFO: public FIFOBase { public: - DynamicFIFO(size_t maxCapacity): FIFOBase(values.data(), maxCapacity), - values(maxCapacity) {}; + DynamicFIFO(size_t maxCapacity): FIFOBase(nullptr, maxCapacity), + fifoVector(maxCapacity) { + // trying to pass the pointer of the uninitialized vector + // to the FIFOBase constructor directly lead to a super evil bug. + // So we do it like this now. + this->setData(fifoVector.data()); + }; + + /** + * @brief Custom copy constructor which prevents setting the + * underlying pointer wrong. + */ + DynamicFIFO(const DynamicFIFO& other): FIFOBase(other), + fifoVector(other.maxCapacity) { + this->setData(fifoVector.data()); + } + private: - std::vector values; + std::vector fifoVector; }; #endif /* FRAMEWORK_CONTAINER_DYNAMICFIFO_H_ */ diff --git a/container/FIFO.h b/container/FIFO.h index ecb218fd..2e332dc2 100644 --- a/container/FIFO.h +++ b/container/FIFO.h @@ -17,10 +17,18 @@ template class FIFO: public FIFOBase { public: - FIFO(): FIFOBase(values.data(), capacity) {}; + FIFO(): FIFOBase(fifoArray.data(), capacity) {}; + + /** + * @brief Custom copy constructor to set pointer correctly. + * @param other + */ + FIFO(const FIFO& other): FIFOBase(other) { + this->setData(fifoArray.data()); + } private: - std::array values; + std::array fifoArray; }; #endif /* FRAMEWORK_CONTAINERS_STATICFIFO_H_ */ diff --git a/container/FIFOBase.h b/container/FIFOBase.h index 082c596a..7f8bde96 100644 --- a/container/FIFOBase.h +++ b/container/FIFOBase.h @@ -44,29 +44,15 @@ public: bool full(); size_t size(); - FIFOBase(const FIFOBase& other): readIndex(other.readIndex), - writeIndex(other.writeIndex), currentSize(other.currentSize), - maxCapacity(other.maxCapacity) { - std::memcpy(values, other.values, sizeof(T) * currentSize); - } - - FIFOBase& operator=(const FIFOBase& other) { - if(&other == this) - return *this; - maxCapacity = other.maxCapacity; - readIndex = other.readIndex; - writeIndex = other.writeIndex; - currentSize = other.currentSize; - std::memcpy(values, other.values, sizeof(T) * currentSize); - return *this; - } size_t getMaxCapacity() const; -private: - T* values; +protected: + void setData(T* data); size_t maxCapacity = 0; + T* values; + size_t readIndex = 0; size_t writeIndex = 0; size_t currentSize = 0; diff --git a/container/FIFOBase.tpp b/container/FIFOBase.tpp index 48a060ff..c73e9d59 100644 --- a/container/FIFOBase.tpp +++ b/container/FIFOBase.tpp @@ -7,7 +7,7 @@ template inline FIFOBase::FIFOBase(T* values, const size_t maxCapacity): - values(values), maxCapacity(maxCapacity) {}; + maxCapacity(maxCapacity), values(values){}; template inline ReturnValue_t FIFOBase::insert(T value) { @@ -78,4 +78,10 @@ inline size_t FIFOBase::getMaxCapacity() const { return maxCapacity; } + +template +inline void FIFOBase::setData(T *data) { + this->values = data; +} + #endif diff --git a/tmtcservices/PusParser.h b/tmtcservices/PusParser.h index 88570bb2..d59673f6 100644 --- a/tmtcservices/PusParser.h +++ b/tmtcservices/PusParser.h @@ -63,11 +63,12 @@ public: * @return */ indexSizePair getNextFifoPair(); -private: - //! A FIFO is used to store information about multiple PUS packets - //! inside the receive buffer. The maximum number of entries is defined - //! by the first constructor argument. +private: + /** A FIFO is used to store information about multiple PUS packets + * inside the receive buffer. The maximum number of entries is defined + * by the first constructor argument. + */ DynamicFIFO indexSizePairFIFO; bool storeSplitPackets = false;