From 3eca16ff785b928f7015444d987a5dbed0ce99dd Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Thu, 4 Jun 2020 20:59:52 +0200 Subject: [PATCH 01/10] removed exit clause --- objectmanager/ObjectManagerIF.h | 1 - 1 file changed, 1 deletion(-) diff --git a/objectmanager/ObjectManagerIF.h b/objectmanager/ObjectManagerIF.h index 3575fc16..b523727c 100644 --- a/objectmanager/ObjectManagerIF.h +++ b/objectmanager/ObjectManagerIF.h @@ -93,7 +93,6 @@ T* ObjectManagerIF::get( object_id_t id ) { if(objectManager == nullptr) { sif::error << "ObjectManagerIF: Global object manager has not " "been initialized yet!" << std::endl; - std::exit(0); } SystemObjectIF* temp = this->getSystemObject(id); return dynamic_cast(temp); From 71487d60ca524042c95dffdf1582252f028f35e2 Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Thu, 4 Jun 2020 21:01:09 +0200 Subject: [PATCH 02/10] onj managerIF update --- objectmanager/ObjectManagerIF.h | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/objectmanager/ObjectManagerIF.h b/objectmanager/ObjectManagerIF.h index b523727c..90a1ce3b 100644 --- a/objectmanager/ObjectManagerIF.h +++ b/objectmanager/ObjectManagerIF.h @@ -1,12 +1,5 @@ -/** - * @file ObjectManagerIF.h - * @brief This file contains the implementation of the ObjectManagerIF interface - * @date 19.09.2012 - * @author Bastian Baetz - */ - -#ifndef OBJECTMANAGERIF_H_ -#define OBJECTMANAGERIF_H_ +#ifndef FRAMEWORK_OBJECTMANAGER_OBJECTMANAGERIF_H_ +#define FRAMEWORK_OBJECTMANAGER_OBJECTMANAGERIF_H_ #include #include @@ -20,7 +13,8 @@ * inserted, removed and retrieved from the list. On getting the * object, the call checks if the object implements the requested * interface. - * \ingroup system_objects + * @author Bastian Baetz + * @ingroup system_objects */ class ObjectManagerIF : public HasReturnvaluesIF { public: From 849053b830d92ccd463779569cb6c450033b47c1 Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Thu, 4 Jun 2020 21:04:24 +0200 Subject: [PATCH 03/10] small fix for object manager.cpp --- objectmanager/ObjectManager.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/objectmanager/ObjectManager.cpp b/objectmanager/ObjectManager.cpp index 1912715d..67cf5c1c 100644 --- a/objectmanager/ObjectManager.cpp +++ b/objectmanager/ObjectManager.cpp @@ -23,8 +23,8 @@ ReturnValue_t ObjectManager::insert( object_id_t id, SystemObjectIF* object) { } else { sif::error << "ObjectManager::insert: Object id " << std::hex << (int)id << std::dec << " is already in use!" << std::endl; - exit(0); //This is very severe and difficult to handle in other places. - return this->INSERTION_FAILED; + //This is very severe and difficult to handle in other places. + std::exit(INSERTION_FAILED); } } From 569724843e7196d5c7ca0b324cd575b6aa9aa9ba Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Thu, 4 Jun 2020 21:11:49 +0200 Subject: [PATCH 04/10] object manager improvements --- objectmanager/ObjectManager.cpp | 34 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/objectmanager/ObjectManager.cpp b/objectmanager/ObjectManager.cpp index 67cf5c1c..3e0bfed6 100644 --- a/objectmanager/ObjectManager.cpp +++ b/objectmanager/ObjectManager.cpp @@ -2,21 +2,19 @@ #include #include -ObjectManager::ObjectManager( void (*setProducer)() ) : produceObjects(setProducer) { +ObjectManager::ObjectManager( void (*setProducer)() ): + produceObjects(setProducer) { //There's nothing special to do in the constructor. } ObjectManager::~ObjectManager() { - std::map::iterator it; - for (it = this->objectList.begin(); it != this->objectList.end(); it++) { - delete it->second; - } + // Map is STL object and deletes its own pointers. } ReturnValue_t ObjectManager::insert( object_id_t id, SystemObjectIF* object) { - bool insert_return = this->objectList.insert( std::pair< object_id_t, SystemObjectIF* >( id, object ) ).second; - if (insert_return == true) { + auto returnPair = objectList.emplace(id, object); + if (returnPair.second) { // sif::debug << "ObjectManager::insert: Object " << std::hex // << (int)id << std::dec << " inserted." << std::endl; return this->RETURN_OK; @@ -44,18 +42,15 @@ ReturnValue_t ObjectManager::remove( object_id_t id ) { SystemObjectIF* ObjectManager::getSystemObject( object_id_t id ) { - std::map::iterator it = this->objectList.find( id ); - if (it == this->objectList.end() ) { - //Changed for testing different method. -// SystemObjectIF* object = this->produceObjects( id ); -// return object; - return NULL; + auto listIter = this->objectList.find( id ); + if (listIter == this->objectList.end() ) { + return nullptr; } else { - return it->second; + return listIter->second; } } -ObjectManager::ObjectManager( ) : produceObjects(NULL) { +ObjectManager::ObjectManager() : produceObjects(nullptr) { } @@ -63,7 +58,7 @@ void ObjectManager::initialize() { this->produceObjects(); ReturnValue_t return_value = RETURN_FAILED; uint32_t error_count = 0; - for (std::map::iterator it = this->objectList.begin(); it != objectList.end(); it++ ) { + for (auto it = this->objectList.begin(); it != objectList.end(); it++ ) { return_value = it->second->initialize(); if ( return_value != RETURN_OK ) { object_id_t var = it->first; @@ -79,10 +74,11 @@ void ObjectManager::initialize() { } //Init was successful. Now check successful interconnections. error_count = 0; - for (std::map::iterator it = this->objectList.begin(); it != objectList.end(); it++ ) { - return_value = it->second->checkObjectConnections(); + for (auto listIter = this->objectList.begin(); + listIter != objectList.end(); listIter++ ) { + return_value = listIter->second->checkObjectConnections(); if ( return_value != RETURN_OK ) { - sif::error << "Object " << std::hex << (int) it->first + sif::error << "Object " << std::hex << (int) listIter->first << " connection check failed with code 0x" << return_value << std::dec << std::endl; error_count++; From d423c001156916fe62b3113c539dee9931f359fb Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Thu, 4 Jun 2020 21:14:35 +0200 Subject: [PATCH 05/10] additional nullptr check --- objectmanager/ObjectManager.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/objectmanager/ObjectManager.cpp b/objectmanager/ObjectManager.cpp index 3e0bfed6..a5895098 100644 --- a/objectmanager/ObjectManager.cpp +++ b/objectmanager/ObjectManager.cpp @@ -55,6 +55,11 @@ ObjectManager::ObjectManager() : produceObjects(nullptr) { } void ObjectManager::initialize() { + if(produceObjects == nullptr) { + sif::error << "ObjectManager: Passed produceObjects functions is" + "nullptr!" << std::endl; + return; + } this->produceObjects(); ReturnValue_t return_value = RETURN_FAILED; uint32_t error_count = 0; From e4944a067c3624a669c4703bfa0d5dce5f1b4119 Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Thu, 4 Jun 2020 21:24:44 +0200 Subject: [PATCH 06/10] change made was wrong (pointers in map are not deleted!) --- objectmanager/ObjectManager.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/objectmanager/ObjectManager.cpp b/objectmanager/ObjectManager.cpp index a5895098..b12c66d9 100644 --- a/objectmanager/ObjectManager.cpp +++ b/objectmanager/ObjectManager.cpp @@ -9,7 +9,9 @@ ObjectManager::ObjectManager( void (*setProducer)() ): ObjectManager::~ObjectManager() { - // Map is STL object and deletes its own pointers. + for (auto const& iter : objectList) { + delete iter.second; + } } ReturnValue_t ObjectManager::insert( object_id_t id, SystemObjectIF* object) { @@ -99,7 +101,7 @@ void ObjectManager::initialize() { void ObjectManager::printList() { std::map::iterator it; sif::debug << "ObjectManager: Object List contains:" << std::endl; - for (it = this->objectList.begin(); it != this->objectList.end(); it++) { - sif::debug << std::hex << it->first << " | " << it->second << std::endl; + for (auto const& it : objectList) { + sif::debug << std::hex << it.first << " | " << it.second << std::endl; } } From dd193fd64d302e5592dfddc8a4395101d921b61b Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Thu, 4 Jun 2020 21:27:37 +0200 Subject: [PATCH 07/10] obj manager.cpp improvements --- objectmanager/ObjectManager.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/objectmanager/ObjectManager.cpp b/objectmanager/ObjectManager.cpp index b12c66d9..2f47950f 100644 --- a/objectmanager/ObjectManager.cpp +++ b/objectmanager/ObjectManager.cpp @@ -65,10 +65,10 @@ void ObjectManager::initialize() { this->produceObjects(); ReturnValue_t return_value = RETURN_FAILED; uint32_t error_count = 0; - for (auto it = this->objectList.begin(); it != objectList.end(); it++ ) { - return_value = it->second->initialize(); + for (auto const& it : objectList) { + return_value = it.second->initialize(); if ( return_value != RETURN_OK ) { - object_id_t var = it->first; + object_id_t var = it.first; sif::error << "Object 0x" << std::hex << std::setw(8) << std::setfill('0')<< var << " failed to initialize " << "with code 0x" << return_value << std::dec << std::endl; @@ -81,11 +81,10 @@ void ObjectManager::initialize() { } //Init was successful. Now check successful interconnections. error_count = 0; - for (auto listIter = this->objectList.begin(); - listIter != objectList.end(); listIter++ ) { - return_value = listIter->second->checkObjectConnections(); + for (auto const& it : objectList) { + return_value = it.second->checkObjectConnections(); if ( return_value != RETURN_OK ) { - sif::error << "Object " << std::hex << (int) listIter->first + sif::error << "Object " << std::hex << (int) it.first << " connection check failed with code 0x" << return_value << std::dec << std::endl; error_count++; From 904721cc3638ffac5a1c4758e84395726d2a415b Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Fri, 5 Jun 2020 13:44:11 +0200 Subject: [PATCH 08/10] improved pool entry iF --- datapool/PoolEntry.cpp | 38 ++++++++------ datapool/PoolEntry.h | 115 ++++++++++++++++++++++++++--------------- datapool/PoolEntryIF.h | 32 ++++++------ 3 files changed, 110 insertions(+), 75 deletions(-) diff --git a/datapool/PoolEntry.cpp b/datapool/PoolEntry.cpp index d07c516d..a87a6c4e 100644 --- a/datapool/PoolEntry.cpp +++ b/datapool/PoolEntry.cpp @@ -1,30 +1,37 @@ #include #include +#include +#include template -PoolEntry::PoolEntry(std::initializer_list initValue, uint8_t set_length, - uint8_t set_valid ) : length(set_length), valid(set_valid) { +PoolEntry::PoolEntry(std::initializer_list initValue, uint8_t setLength, + bool setValid ) : length(setLength), valid(setValid) { this->address = new T[this->length]; if(initValue.size() == 0) { - memset(this->address, 0, this->getByteSize()); + std::memset(this->address, 0, this->getByteSize()); + } + else if (initValue.size() != setLength){ + sif::warning << "PoolEntry: setLength is not equal to initializer list" + "length! Performing zero initialization with given setLength" + << std::endl; + std::memset(this->address, 0, this->getByteSize()); } else { - memcpy(this->address, initValue.begin(), this->getByteSize()); + std::copy(initValue.begin(), initValue.end(), this->address); } } template -PoolEntry::PoolEntry( T* initValue, uint8_t set_length, uint8_t set_valid ) : - length(set_length), valid(set_valid) { +PoolEntry::PoolEntry( T* initValue, uint8_t setLength, bool setValid ) : + length(setLength), valid(setValid) { this->address = new T[this->length]; - if (initValue != NULL) { - memcpy(this->address, initValue, this->getByteSize() ); + if (initValue != nullptr) { + std::memcpy(this->address, initValue, this->getByteSize() ); } else { - memset(this->address, 0, this->getByteSize() ); + std::memset(this->address, 0, this->getByteSize() ); } } - //As the data pool is global, this dtor is only be called on program exit. //Warning! Never copy pool entries! template @@ -48,21 +55,20 @@ void* PoolEntry::getRawData() { } template -void PoolEntry::setValid( uint8_t isValid ) { +void PoolEntry::setValid(bool isValid) { this->valid = isValid; } template -uint8_t PoolEntry::getValid() { +bool PoolEntry::getValid() { return valid; } template void PoolEntry::print() { - for (uint8_t size = 0; size < this->length; size++ ) { - sif::debug << "| " << std::hex << (double)this->address[size] - << (this->valid? " (valid) " : " (invalid) "); - } + sif::debug << "Pool Entry Validity: " << + (this->valid? " (valid) " : " (invalid) ") << std::endl; + printer::print(reinterpret_cast(address), length); sif::debug << std::dec << std::endl; } diff --git a/datapool/PoolEntry.h b/datapool/PoolEntry.h index 4971ee45..1a22bb63 100644 --- a/datapool/PoolEntry.h +++ b/datapool/PoolEntry.h @@ -1,95 +1,126 @@ -#ifndef POOLENTRY_H_ -#define POOLENTRY_H_ - +#ifndef FRAMEWORK_DATAPOOL_POOLENTRY_H_ +#define FRAMEWORK_DATAPOOL_POOLENTRY_H_ #include -#include -#include + #include #include +#include + /** * @brief This is a small helper class that defines a single data pool entry. * @details * The helper is used to store all information together with the data as a - * single data pool entry.The content's type is defined by the template argument. - * It is prepared for use with plain old data types, but may be extended to - * complex types if necessary. It can be initialized with a certain value, - * size and validity flag. It holds a pointer to the real data and offers - * methods to access this data and to acquire additional information - * (such as validity and array/byte size). It is NOT intended to be used - * outside the DataPool class. - * @author Bastian Baetz - * @ingroup data_pool + * single data pool entry. The content's type is defined by the template + * argument. * + * It is prepared for use with plain old data types, but may be + * extended to complex types if necessary. It can be initialized with a + * certain value, size and validity flag. + * + * It holds a pointer to the real data and offers methods to access this data + * and to acquire additional information (such as validity and array/byte size). + * It is NOT intended to be used outside DataPool implementations as it performs + * dynamic memory allocation. + * + * @ingroup data_pool */ template class PoolEntry : public PoolEntryIF { public: static_assert(not std::is_same::value, - "Do not use boolean for the PoolEntry type, use uint8_t instead!" - "Warum? Darum :-)"); + "Do not use boolean for the PoolEntry type, use uint8_t " + "instead! The ECSS standard defines a boolean as a one bit " + "field. Therefore it is preferred to store a boolean as an " + "uint8_t"); /** * @brief In the classe's constructor, space is allocated on the heap and * potential init values are copied to that space. - * @param initValue Initializer list with values to initialize with - * @param set_length Defines the array length of this entry. - * @param set_valid Sets the initialization flag. It is invalid (0) by default. + * @details + * Not passing any arguments will initialize an non-array pool entry + * (setLength = 1) with an initial invalid state. + * Please note that if an initializer list is passed, the correct + * corresponding length should be passed too, otherwise a zero + * initialization will be performed with the given setLength. + * @param initValue + * Initializer list with values to initialize with, for example {0,0} to + * initialize the two entries to zero. + * @param setLength + * Defines the array length of this entry. Should be equal to the + * intializer list length. + * @param setValid + * Sets the initialization flag. It is invalid by default. */ - PoolEntry( std::initializer_list initValue = {}, uint8_t set_length = 1, uint8_t set_valid = 0 ); + PoolEntry(std::initializer_list initValue = {}, uint8_t setLength = 1, + bool setValid = false); /** * @brief In the classe's constructor, space is allocated on the heap and * potential init values are copied to that space. - * @param initValue A pointer to the single value or array that holds the init value. - * With the default value (NULL), the entry is initalized with all 0. - * @param set_length Defines the array length of this entry. - * @param set_valid Sets the initialization flag. It is invalid (0) by default. + * @param initValue + * A pointer to the single value or array that holds the init value. + * With the default value (nullptr), the entry is initalized with all 0. + * @param setLength + * Defines the array length of this entry. + * @param setValid + * Sets the initialization flag. It is invalid by default. */ - PoolEntry( T* initValue = NULL, uint8_t set_length = 1, uint8_t set_valid = 0 ); + PoolEntry(T* initValue, uint8_t setLength = 1, bool setValid = false); + + //! Explicitely deleted copy ctor, copying is not allowed! + PoolEntry(const PoolEntry&) = delete; + //! Explicitely deleted copy assignment, copying is not allowed! + PoolEntry& operator=(const PoolEntry&) = delete; /** - * \brief The allocated memory for the variable is freed in the destructor. - * \details As the data pool is global, this dtor is only called on program exit. - * PoolEntries shall never be copied, as a copy might delete the variable on the heap. + * @brief The allocated memory for the variable is freed + * in the destructor. + * @details + * As the data pool is global, this dtor is only called on program exit. + * PoolEntries shall never be copied, as a copy might delete the variable + * on the heap. */ ~PoolEntry(); + /** - * \brief This is the address pointing to the allocated memory. + * @brief This is the address pointing to the allocated memory. */ T* address; /** - * \brief This attribute stores the length information. + * @brief This attribute stores the length information. */ uint8_t length; /** - * \brief Here, the validity information for a variable is stored. + * @brief Here, the validity information for a variable is stored. * Every entry (single variable or vector) has one valid flag. */ uint8_t valid; /** - * \brief getSize returns the array size of the entry. - * \details A single parameter has size 1. + * @brief getSize returns the array size of the entry. + * @details A single parameter has size 1. */ uint8_t getSize(); /** - * \brief This operation returns the size in bytes. - * \details The size is calculated by sizeof(type) * array_size. + * @brief This operation returns the size in bytes. + * @details The size is calculated by sizeof(type) * array_size. */ uint16_t getByteSize(); /** - * \brief This operation returns a the address pointer casted to void*. + * @brief This operation returns a the address pointer casted to void*. */ void* getRawData(); /** - * \brief This method allows to set the valid information of the pool entry. + * @brief This method allows to set the valid information + * of the pool entry. */ - void setValid( uint8_t isValid ); + void setValid( bool isValid ); /** - * \brief This method allows to get the valid information of the pool entry. + * @brief This method allows to get the valid information + * of the pool entry. */ - uint8_t getValid(); + bool getValid(); /** - * \brief This is a debug method that prints all values and the valid information to the screen. - * It prints all array entries in a row. + * @brief This is a debug method that prints all values and the valid + * information to the screen. It prints all array entries in a row. */ void print(); diff --git a/datapool/PoolEntryIF.h b/datapool/PoolEntryIF.h index d24d553a..a075436e 100644 --- a/datapool/PoolEntryIF.h +++ b/datapool/PoolEntryIF.h @@ -1,5 +1,5 @@ -#ifndef POOLENTRYIF_H_ -#define POOLENTRYIF_H_ +#ifndef FRAMEWORK_DATAPOOL_POOLENTRYIF_H_ +#define FRAMEWORK_DATAPOOL_POOLENTRYIF_H_ #include #include @@ -9,55 +9,53 @@ * single data pool entry. * @details * The interface provides methods to determine the size and the validity - * information of a value. It also defines a method to receive a pointer to - * the raw data content. It is mainly used by DataPool itself, but also as a + * information of a value. It also defines a method to receive a pointer to the + * raw data content. It is mainly used by DataPool itself, but also as a * return pointer. - * @author Bastian Baetz + * + * @author Bastian Baetz * @ingroup data_pool + * */ class PoolEntryIF { public: /** * @brief This is an empty virtual destructor, - * as it is proposed for C++ interfaces. + * as it is required for C++ interfaces. */ - virtual ~PoolEntryIF() {} - + virtual ~PoolEntryIF() { + } /** * @brief getSize returns the array size of the entry. * A single variable parameter has size 1. */ virtual uint8_t getSize() = 0; - /** * @brief This operation returns the size in bytes, which is calculated by * sizeof(type) * array_size. */ virtual uint16_t getByteSize() = 0; - /** * @brief This operation returns a the address pointer casted to void*. */ virtual void* getRawData() = 0; - /** * @brief This method allows to set the valid information of the pool entry. */ - virtual void setValid(uint8_t isValid) = 0; - + virtual void setValid(bool isValid) = 0; /** * @brief This method allows to set the valid information of the pool entry. */ - virtual uint8_t getValid() = 0; - + virtual bool getValid() = 0; /** * @brief This is a debug method that prints all values and the valid * information to the screen. It prints all array entries in a row. + * @details + * Also displays whether the pool entry is valid or invalid. */ virtual void print() = 0; - /** - * @brief Returns the type of the entry. + * Returns the type of the entry. */ virtual Type getType() = 0; }; From dd210e99af8ece3866bc898ec3daa631d90b8d9d Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Fri, 5 Jun 2020 13:45:18 +0200 Subject: [PATCH 09/10] additional include which was missing --- datapoolglob/PoolRawAccess.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/datapoolglob/PoolRawAccess.cpp b/datapoolglob/PoolRawAccess.cpp index cc04f9b9..b10ee04c 100644 --- a/datapoolglob/PoolRawAccess.cpp +++ b/datapoolglob/PoolRawAccess.cpp @@ -3,6 +3,8 @@ #include #include +#include + PoolRawAccess::PoolRawAccess(uint32_t set_id, uint8_t setArrayEntry, DataSetIF* dataSet, ReadWriteMode_t setReadWriteMode) : dataPoolId(set_id), arrayEntry(setArrayEntry), valid(false), From d7036edb94e7d64626e3cacf3d205c2b9db143a9 Mon Sep 17 00:00:00 2001 From: "Robin.Mueller" Date: Fri, 5 Jun 2020 15:53:20 +0200 Subject: [PATCH 10/10] hotfix --- serialize/SerialBufferAdapter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/serialize/SerialBufferAdapter.cpp b/serialize/SerialBufferAdapter.cpp index 3df2abe9..e13e7eb6 100644 --- a/serialize/SerialBufferAdapter.cpp +++ b/serialize/SerialBufferAdapter.cpp @@ -82,7 +82,7 @@ ReturnValue_t SerialBufferAdapter::deSerialize(const uint8_t** buffer, } } //No Else If, go on with buffer - if (bufferLength >= *size) { + if (bufferLength <= *size) { *size -= bufferLength; memcpy(m_buffer, *buffer, bufferLength); (*buffer) += bufferLength;