From 371ff931bfb1284960b6a13716fdd34251ab8ed8 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Wed, 26 Jan 2022 12:11:52 +0100 Subject: [PATCH 1/7] Linux CommandExecutor The CommandExecutor helper class can execute shell commands in blocking and non-blocking mode This class is able to execute processes by using the Linux popen call. It also has the capability of writing the read output of a process into a provided ring buffer. The executor works by first loading the command which should be executed and specifying whether it should be executed blocking or non-blocking. After that, execution can be started with the execute call. Using non-blocking mode allows to execute commands which might take a longer time in the background, and allowing the user thread to check completion status with the check function Moved to HAL like requested in code review and unit tested with failing commands as well. Also, Linux HAL components are compiled by default now unless explicitely disabled. --- hal/CMakeLists.txt | 8 +- hal/src/fsfw_hal/CMakeLists.txt | 2 +- hal/src/fsfw_hal/linux/CMakeLists.txt | 11 +- hal/src/fsfw_hal/linux/CommandExecutor.cpp | 213 ++++++++++++++++++ hal/src/fsfw_hal/linux/CommandExecutor.h | 135 +++++++++++ hal/src/fsfw_hal/linux/gpio/CMakeLists.txt | 20 +- hal/src/fsfw_hal/linux/i2c/I2cComIF.cpp | 32 ++- hal/src/fsfw_hal/linux/uart/UartComIF.cpp | 46 +++- hal/src/fsfw_hal/linux/uart/UartCookie.cpp | 4 +- scripts/helper.py | 4 +- src/fsfw/FSFW.h.in | 4 + src/fsfw/container/SimpleRingBuffer.h | 15 +- src/fsfw/devicehandlers/AssemblyBase.h | 2 +- .../devicehandlers/DeviceCommunicationIF.h | 7 +- src/fsfw/devicehandlers/DeviceHandlerBase.h | 3 +- src/fsfw/devicehandlers/DeviceHandlerIF.h | 3 +- src/fsfw/osal/linux/CMakeLists.txt | 33 ++- src/fsfw/osal/linux/MessageQueue.cpp | 6 +- src/fsfw/subsystem/SubsystemBase.h | 3 +- tests/src/fsfw_tests/unit/CMakeLists.txt | 1 + tests/src/fsfw_tests/unit/hal/CMakeLists.txt | 3 + .../unit/hal/testCommandExecutor.cpp | 110 +++++++++ 22 files changed, 608 insertions(+), 57 deletions(-) create mode 100644 hal/src/fsfw_hal/linux/CommandExecutor.cpp create mode 100644 hal/src/fsfw_hal/linux/CommandExecutor.h create mode 100644 tests/src/fsfw_tests/unit/hal/CMakeLists.txt create mode 100644 tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp diff --git a/hal/CMakeLists.txt b/hal/CMakeLists.txt index 7a9a0ffa..424a012d 100644 --- a/hal/CMakeLists.txt +++ b/hal/CMakeLists.txt @@ -3,7 +3,13 @@ cmake_minimum_required(VERSION 3.13) # Can also be changed by upper CMakeLists.txt file find_library(LIB_FSFW_NAME fsfw REQUIRED) -option(FSFW_HAL_ADD_LINUX "Add the Linux HAL to the sources. Required gpiod library" OFF) +option(FSFW_HAL_ADD_LINUX "Add the Linux HAL to the sources. Requires gpiod library" OFF) +# On by default for now because I did not have an issue including and compiling those files +# and libraries on a Desktop Linux system and the primary target of the FSFW is still embedded +# Linux. The only exception from this is the gpiod library which requires a dedicated installation, +# but CMake is able to determine whether this library is installed with find_library. +option(FSFW_HAL_LINUX_ADD_PERIPHERAL_DRIVERS "Add peripheral drivers for embedded Linux" ON) + option(FSFW_HAL_ADD_RASPBERRY_PI "Add Raspberry Pi specific code to the sources" OFF) option(FSFW_HAL_ADD_STM32H7 "Add the STM32H7 HAL to the sources" OFF) option(FSFW_HAL_WARNING_SHADOW_LOCAL_GCC "Enable -Wshadow=local warning in GCC" ON) diff --git a/hal/src/fsfw_hal/CMakeLists.txt b/hal/src/fsfw_hal/CMakeLists.txt index f5901e91..b7559d4b 100644 --- a/hal/src/fsfw_hal/CMakeLists.txt +++ b/hal/src/fsfw_hal/CMakeLists.txt @@ -1,7 +1,7 @@ add_subdirectory(devicehandlers) add_subdirectory(common) -if(FSFW_HAL_ADD_LINUX) +if(UNIX) add_subdirectory(linux) endif() diff --git a/hal/src/fsfw_hal/linux/CMakeLists.txt b/hal/src/fsfw_hal/linux/CMakeLists.txt index 5944b0e5..5ec8af06 100644 --- a/hal/src/fsfw_hal/linux/CMakeLists.txt +++ b/hal/src/fsfw_hal/linux/CMakeLists.txt @@ -4,10 +4,13 @@ endif() target_sources(${LIB_FSFW_NAME} PRIVATE UnixFileGuard.cpp + CommandExecutor.cpp utility.cpp ) -add_subdirectory(gpio) -add_subdirectory(spi) -add_subdirectory(i2c) -add_subdirectory(uart) \ No newline at end of file +if(FSFW_HAL_LINUX_ADD_PERIPHERAL_DRIVERS) + add_subdirectory(gpio) + add_subdirectory(spi) + add_subdirectory(i2c) + add_subdirectory(uart) +endif() diff --git a/hal/src/fsfw_hal/linux/CommandExecutor.cpp b/hal/src/fsfw_hal/linux/CommandExecutor.cpp new file mode 100644 index 00000000..016217e1 --- /dev/null +++ b/hal/src/fsfw_hal/linux/CommandExecutor.cpp @@ -0,0 +1,213 @@ +#include "CommandExecutor.h" + +#include "fsfw/serviceinterface.h" +#include "fsfw/container/SimpleRingBuffer.h" +#include "fsfw/container/DynamicFIFO.h" + +#include + +#include + +CommandExecutor::CommandExecutor(const size_t maxSize): +readVec(maxSize) { + waiter.events = POLLIN; +} + +ReturnValue_t CommandExecutor::load(std::string command, bool blocking, bool printOutput) { + if(state == States::PENDING) { + return COMMAND_PENDING; + } + + currentCmd = command; + this->blocking = blocking; + this->printOutput = printOutput; + if(state == States::IDLE) { + state = States::COMMAND_LOADED; + } + return HasReturnvaluesIF::RETURN_OK; +} + +ReturnValue_t CommandExecutor::execute() { + if(state == States::IDLE) { + return NO_COMMAND_LOADED_OR_PENDING; + } + else if(state == States::PENDING) { + return COMMAND_PENDING; + } + currentCmdFile = popen(currentCmd.c_str(), "r"); + if(currentCmdFile == nullptr) { + lastError = errno; + return HasReturnvaluesIF::RETURN_FAILED; + } + if(blocking) { + ReturnValue_t result = executeBlocking(); + state = States::IDLE; + return result; + } + else { + currentFd = fileno(currentCmdFile); + waiter.fd = currentFd; + } + state = States::PENDING; + return HasReturnvaluesIF::RETURN_OK; +} + +ReturnValue_t CommandExecutor::close() { + if(state == States::PENDING) { + // Attempt to close process, irrespective of if it is running or not + if(currentCmdFile != nullptr) { + pclose(currentCmdFile); + } + } + return HasReturnvaluesIF::RETURN_OK; +} + +void CommandExecutor::printLastError(std::string funcName) const { + if(lastError != 0) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 + sif::warning << funcName << " pclose failed with code " << lastError << ": " << + strerror(lastError) << std::endl; +#else + sif::printError("%s pclose failed with code %d: %s\n", + funcName, lastError, strerror(lastError)); +#endif + } +} + +void CommandExecutor::setRingBuffer(SimpleRingBuffer *ringBuffer, + DynamicFIFO* sizesFifo) { + this->ringBuffer = ringBuffer; + this->sizesFifo = sizesFifo; +} + +ReturnValue_t CommandExecutor::check(bool& replyReceived) { + if(blocking) { + return HasReturnvaluesIF::RETURN_OK; + } + switch(state) { + case(States::IDLE): + case(States::COMMAND_LOADED): { + return NO_COMMAND_LOADED_OR_PENDING; + } + case(States::PENDING): { + break; + } + } + + int result = poll(&waiter, 1, 0); + switch(result) { + case(0): { + return HasReturnvaluesIF::RETURN_OK; + break; + } + case(1): { + if (waiter.revents & POLLIN) { + ssize_t readBytes = read(currentFd, readVec.data(), readVec.size()); + if(readBytes == 0) { + // Should not happen +#if FSFW_CPP_OSTREAM_ENABLED == 1 + sif::warning << "CommandExecutor::check: No bytes read " + "after poll event.." << std::endl; +#else + sif::printWarning("CommandExecutor::check: No bytes read after poll event..\n"); +#endif + break; + } + else if(readBytes > 0) { + replyReceived = true; + if(printOutput) { + // It is assumed the command output is line terminated +#if FSFW_CPP_OSTREAM_ENABLED == 1 + sif::info << currentCmd << " | " << readVec.data(); +#else + sif::printInfo("%s | %s", currentCmd, readVec.data()); +#endif + } + if(ringBuffer != nullptr) { + ringBuffer->writeData(reinterpret_cast( + readVec.data()), readBytes); + } + if(sizesFifo != nullptr) { + if(not sizesFifo->full()) { + sizesFifo->insert(readBytes); + } + } + } + else { + // Should also not happen +#if FSFW_CPP_OSTREAM_ENABLED == 1 + sif::warning << "CommandExecutor::check: Error " << errno << ": " << + strerror(errno) << std::endl; +#else + sif::printWarning("CommandExecutor::check: Error %d: %s\n", errno, strerror(errno)); +#endif + } + } + if(waiter.revents & POLLERR) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 + sif::warning << "CommandExecuter::check: Poll error" << std::endl; +#else + sif::printWarning("CommandExecuter::check: Poll error\n"); +#endif + return COMMAND_ERROR; + } + if(waiter.revents & POLLHUP) { + result = pclose(currentCmdFile); + ReturnValue_t retval = EXECUTION_FINISHED; + if(result != 0) { + lastError = result; + retval = HasReturnvaluesIF::RETURN_FAILED; + } + state = States::IDLE; + currentCmdFile = nullptr; + currentFd = 0; + return retval; + } + break; + } + } + return HasReturnvaluesIF::RETURN_OK; +} + +void CommandExecutor::reset() { + CommandExecutor::close(); + currentCmdFile = nullptr; + currentFd = 0; + state = States::IDLE; +} + +int CommandExecutor::getLastError() const { + // See: https://stackoverflow.com/questions/808541/any-benefit-in-using-wexitstatus-macro-in-c-over-division-by-256-on-exit-statu + return WEXITSTATUS(this->lastError); +} + +CommandExecutor::States CommandExecutor::getCurrentState() const { + return state; +} + +ReturnValue_t CommandExecutor::executeBlocking() { + while(fgets(readVec.data(), readVec.size(), currentCmdFile) != nullptr) { + std::string output(readVec.data()); + if(printOutput) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 + sif::info << currentCmd << " | " << output; +#else + sif::printInfo("%s | %s", currentCmd, output); +#endif + } + if(ringBuffer != nullptr) { + ringBuffer->writeData(reinterpret_cast(output.data()), output.size()); + } + if(sizesFifo != nullptr) { + if(not sizesFifo->full()) { + sizesFifo->insert(output.size()); + } + } + } + int result = pclose(currentCmdFile); + if(result != 0) { + lastError = result; + return HasReturnvaluesIF::RETURN_FAILED; + } + return HasReturnvaluesIF::RETURN_OK; +} diff --git a/hal/src/fsfw_hal/linux/CommandExecutor.h b/hal/src/fsfw_hal/linux/CommandExecutor.h new file mode 100644 index 00000000..895bd943 --- /dev/null +++ b/hal/src/fsfw_hal/linux/CommandExecutor.h @@ -0,0 +1,135 @@ +#ifndef FSFW_SRC_FSFW_OSAL_LINUX_COMMANDEXECUTOR_H_ +#define FSFW_SRC_FSFW_OSAL_LINUX_COMMANDEXECUTOR_H_ + +#include "fsfw/returnvalues/HasReturnvaluesIF.h" +#include "fsfw/returnvalues/FwClassIds.h" + +#include + +#include +#include + +class SimpleRingBuffer; +template class DynamicFIFO; + +/** + * @brief Helper class to execute shell commands in blocking and non-blocking mode + * @details + * This class is able to execute processes by using the Linux popen call. It also has the + * capability of writing the read output of a process into a provided ring buffer. + * + * The executor works by first loading the command which should be executed and specifying + * whether it should be executed blocking or non-blocking. After that, execution can be started + * with the execute command. In blocking mode, the execute command will block until the command + * has finished + */ +class CommandExecutor { +public: + enum class States { + IDLE, + COMMAND_LOADED, + PENDING + }; + + static constexpr uint8_t CLASS_ID = CLASS_ID::LINUX_OSAL; + + //! [EXPORT] : [COMMENT] Execution of the current command has finished + static constexpr ReturnValue_t EXECUTION_FINISHED = + HasReturnvaluesIF::makeReturnCode(CLASS_ID, 0); + + //! [EXPORT] : [COMMENT] Command is pending. This will also be returned if the user tries + //! to load another command but a command is still pending + static constexpr ReturnValue_t COMMAND_PENDING = + HasReturnvaluesIF::makeReturnCode(CLASS_ID, 1); + //! [EXPORT] : [COMMENT] Some bytes have been read from the executing process + static constexpr ReturnValue_t BYTES_READ = + HasReturnvaluesIF::makeReturnCode(CLASS_ID, 2); + //! [EXPORT] : [COMMENT] Command execution failed + static constexpr ReturnValue_t COMMAND_ERROR = + HasReturnvaluesIF::makeReturnCode(CLASS_ID, 3); + //! [EXPORT] : [COMMENT] + static constexpr ReturnValue_t NO_COMMAND_LOADED_OR_PENDING = + HasReturnvaluesIF::makeReturnCode(CLASS_ID, 4); + static constexpr ReturnValue_t PCLOSE_CALL_ERROR = + HasReturnvaluesIF::makeReturnCode(CLASS_ID, 6); + + /** + * Constructor. Is initialized with maximum size of internal buffer to read data from the + * executed process. + * @param maxSize + */ + CommandExecutor(const size_t maxSize); + + /** + * Load a new command which should be executed + * @param command + * @param blocking + * @param printOutput + * @return + */ + ReturnValue_t load(std::string command, bool blocking, bool printOutput = true); + /** + * Execute the loaded command. + * @return + * - In blocking mode, it will return RETURN_FAILED if + * the result of the system call was not 0. The error value can be accessed using + * getLastError + * - In non-blocking mode, this call will start + * the execution and then return RETURN_OK + */ + ReturnValue_t execute(); + /** + * Only used in non-blocking mode. Checks the currently running command. + * @param bytesRead Will be set to the number of bytes read, if bytes have been read + * @return + * - BYTES_READ if bytes have been read from the executing process. It is recommended to call + * check again after this + * - RETURN_OK execution is pending, but no bytes have been read from the executing process + * - RETURN_FAILED if execution has failed, error value can be accessed using getLastError + * - EXECUTION_FINISHED if the process was executed successfully + * - NO_COMMAND_LOADED_OR_PENDING self-explanatory + * - COMMAND_ERROR internal poll error + */ + ReturnValue_t check(bool& replyReceived); + /** + * Abort the current command. Should normally not be necessary, check can be used to find + * out whether command execution was successful + * @return RETURN_OK + */ + ReturnValue_t close(); + + States getCurrentState() const; + int getLastError() const; + void printLastError(std::string funcName) const; + + /** + * Assign a ring buffer and a FIFO which will be filled by the executor with the output + * read from the started process + * @param ringBuffer + * @param sizesFifo + */ + void setRingBuffer(SimpleRingBuffer* ringBuffer, DynamicFIFO* sizesFifo); + + /** + * Reset the executor. This calls close internally and then reset the state machine so new + * commands can be loaded and executed + */ + void reset(); +private: + std::string currentCmd; + bool blocking = true; + FILE* currentCmdFile = nullptr; + int currentFd = 0; + bool printOutput = true; + std::vector readVec; + struct pollfd waiter {}; + SimpleRingBuffer* ringBuffer = nullptr; + DynamicFIFO* sizesFifo = nullptr; + + States state = States::IDLE; + int lastError = 0; + + ReturnValue_t executeBlocking(); +}; + +#endif /* FSFW_SRC_FSFW_OSAL_LINUX_COMMANDEXECUTOR_H_ */ diff --git a/hal/src/fsfw_hal/linux/gpio/CMakeLists.txt b/hal/src/fsfw_hal/linux/gpio/CMakeLists.txt index 7083f70d..b1609850 100644 --- a/hal/src/fsfw_hal/linux/gpio/CMakeLists.txt +++ b/hal/src/fsfw_hal/linux/gpio/CMakeLists.txt @@ -1,12 +1,16 @@ -target_sources(${LIB_FSFW_NAME} PRIVATE - LinuxLibgpioIF.cpp -) - # This abstraction layer requires the gpiod library. You can install this library # with "sudo apt-get install -y libgpiod-dev". If you are cross-compiling, you need # to install the package before syncing the sysroot to your host computer. -find_library(LIB_GPIO gpiod REQUIRED) +find_library(LIB_GPIO gpiod) + +if(${LIB_GPIO} MATCHES LIB_GPIO-NOTFOUND) + message(STATUS "gpiod library not found, not linking against it") +else() + target_sources(${LIB_FSFW_NAME} PRIVATE + LinuxLibgpioIF.cpp + ) + target_link_libraries(${LIB_FSFW_NAME} PRIVATE + ${LIB_GPIO} + ) +endif() -target_link_libraries(${LIB_FSFW_NAME} PRIVATE - ${LIB_GPIO} -) \ No newline at end of file diff --git a/hal/src/fsfw_hal/linux/i2c/I2cComIF.cpp b/hal/src/fsfw_hal/linux/i2c/I2cComIF.cpp index 98d9a510..fd5e8454 100644 --- a/hal/src/fsfw_hal/linux/i2c/I2cComIF.cpp +++ b/hal/src/fsfw_hal/linux/i2c/I2cComIF.cpp @@ -1,8 +1,10 @@ +#include "fsfw/FSFW.h" + #include "fsfw_hal/linux/i2c/I2cComIF.h" #include "fsfw_hal/linux/utility.h" #include "fsfw_hal/linux/UnixFileGuard.h" -#include "fsfw/serviceinterface/ServiceInterface.h" +#include "fsfw/serviceinterface.h" #include #include @@ -24,12 +26,16 @@ ReturnValue_t I2cComIF::initializeInterface(CookieIF* cookie) { std::string deviceFile; if(cookie == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "I2cComIF::initializeInterface: Invalid cookie!" << std::endl; +#endif return NULLPOINTER; } I2cCookie* i2cCookie = dynamic_cast(cookie); if(i2cCookie == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "I2cComIF::initializeInterface: Invalid I2C cookie!" << std::endl; +#endif return NULLPOINTER; } @@ -41,15 +47,19 @@ ReturnValue_t I2cComIF::initializeInterface(CookieIF* cookie) { I2cInstance i2cInstance = {std::vector(maxReplyLen), 0}; auto statusPair = i2cDeviceMap.emplace(i2cAddress, i2cInstance); if (not statusPair.second) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "I2cComIF::initializeInterface: Failed to insert device with address " << i2cAddress << "to I2C device " << "map" << std::endl; +#endif return HasReturnvaluesIF::RETURN_FAILED; } return HasReturnvaluesIF::RETURN_OK; } +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "I2cComIF::initializeInterface: Device with address " << i2cAddress << "already in use" << std::endl; +#endif return HasReturnvaluesIF::RETURN_FAILED; } @@ -61,8 +71,10 @@ ReturnValue_t I2cComIF::sendMessage(CookieIF *cookie, std::string deviceFile; if(sendData == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "I2cComIF::sendMessage: Send Data is nullptr" << std::endl; +#endif return HasReturnvaluesIF::RETURN_FAILED; } @@ -72,15 +84,19 @@ ReturnValue_t I2cComIF::sendMessage(CookieIF *cookie, I2cCookie* i2cCookie = dynamic_cast(cookie); if(i2cCookie == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "I2cComIF::sendMessage: Invalid I2C Cookie!" << std::endl; +#endif return NULLPOINTER; } address_t i2cAddress = i2cCookie->getAddress(); i2cDeviceMapIter = i2cDeviceMap.find(i2cAddress); if (i2cDeviceMapIter == i2cDeviceMap.end()) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "I2cComIF::sendMessage: i2cAddress of Cookie not " << "registered in i2cDeviceMap" << std::endl; +#endif return HasReturnvaluesIF::RETURN_FAILED; } @@ -94,10 +110,12 @@ ReturnValue_t I2cComIF::sendMessage(CookieIF *cookie, return result; } - if (write(fd, sendData, sendLen) != (int)sendLen) { + if (write(fd, sendData, sendLen) != static_cast(sendLen)) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "I2cComIF::sendMessage: Failed to send data to I2C " "device with error code " << errno << ". Error description: " << strerror(errno) << std::endl; +#endif return HasReturnvaluesIF::RETURN_FAILED; } return HasReturnvaluesIF::RETURN_OK; @@ -119,7 +137,9 @@ ReturnValue_t I2cComIF::requestReceiveMessage(CookieIF *cookie, I2cCookie* i2cCookie = dynamic_cast(cookie); if(i2cCookie == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "I2cComIF::requestReceiveMessage: Invalid I2C Cookie!" << std::endl; +#endif i2cDeviceMapIter->second.replyLen = 0; return NULLPOINTER; } @@ -127,8 +147,10 @@ ReturnValue_t I2cComIF::requestReceiveMessage(CookieIF *cookie, address_t i2cAddress = i2cCookie->getAddress(); i2cDeviceMapIter = i2cDeviceMap.find(i2cAddress); if (i2cDeviceMapIter == i2cDeviceMap.end()) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "I2cComIF::requestReceiveMessage: i2cAddress of Cookie not " << "registered in i2cDeviceMap" << std::endl; +#endif i2cDeviceMapIter->second.replyLen = 0; return HasReturnvaluesIF::RETURN_FAILED; } @@ -156,7 +178,9 @@ ReturnValue_t I2cComIF::requestReceiveMessage(CookieIF *cookie, << requestLen << " bytes" << std::endl; #endif i2cDeviceMapIter->second.replyLen = 0; +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::debug << "I2cComIF::requestReceiveMessage: Read " << readLen << " of " << requestLen << " bytes" << std::endl; +#endif return HasReturnvaluesIF::RETURN_FAILED; } @@ -168,15 +192,19 @@ ReturnValue_t I2cComIF::readReceivedMessage(CookieIF *cookie, uint8_t **buffer, size_t* size) { I2cCookie* i2cCookie = dynamic_cast(cookie); if(i2cCookie == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "I2cComIF::readReceivedMessage: Invalid I2C Cookie!" << std::endl; +#endif return NULLPOINTER; } address_t i2cAddress = i2cCookie->getAddress(); i2cDeviceMapIter = i2cDeviceMap.find(i2cAddress); if (i2cDeviceMapIter == i2cDeviceMap.end()) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "I2cComIF::readReceivedMessage: i2cAddress of Cookie not " << "found in i2cDeviceMap" << std::endl; +#endif return HasReturnvaluesIF::RETURN_FAILED; } *buffer = i2cDeviceMapIter->second.replyBuffer.data(); diff --git a/hal/src/fsfw_hal/linux/uart/UartComIF.cpp b/hal/src/fsfw_hal/linux/uart/UartComIF.cpp index f5754c6e..08ec9aa0 100644 --- a/hal/src/fsfw_hal/linux/uart/UartComIF.cpp +++ b/hal/src/fsfw_hal/linux/uart/UartComIF.cpp @@ -1,8 +1,8 @@ #include "UartComIF.h" -#include "OBSWConfig.h" +#include "fsfw/FSFW.h" #include "fsfw_hal/linux/utility.h" -#include "fsfw/serviceinterface/ServiceInterface.h" +#include "fsfw/serviceinterface.h" #include #include @@ -26,7 +26,9 @@ ReturnValue_t UartComIF::initializeInterface(CookieIF* cookie) { UartCookie* uartCookie = dynamic_cast(cookie); if (uartCookie == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "UartComIF::initializeInterface: Invalid UART Cookie!" << std::endl; +#endif return NULLPOINTER; } @@ -42,14 +44,18 @@ ReturnValue_t UartComIF::initializeInterface(CookieIF* cookie) { UartElements uartElements = {fileDescriptor, std::vector(maxReplyLen), 0}; auto status = uartDeviceMap.emplace(deviceFile, uartElements); if (status.second == false) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::initializeInterface: Failed to insert device " << deviceFile << "to UART device map" << std::endl; +#endif return RETURN_FAILED; } } else { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::initializeInterface: UART device " << deviceFile << " already in use" << std::endl; +#endif return RETURN_FAILED; } @@ -70,15 +76,19 @@ int UartComIF::configureUartPort(UartCookie* uartCookie) { int fd = open(deviceFile.c_str(), flags); if (fd < 0) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::configureUartPort: Failed to open uart " << deviceFile << "with error code " << errno << strerror(errno) << std::endl; +#endif return fd; } /* Read in existing settings */ if(tcgetattr(fd, &options) != 0) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::configureUartPort: Error " << errno << "from tcgetattr: " << strerror(errno) << std::endl; +#endif return fd; } @@ -99,8 +109,10 @@ int UartComIF::configureUartPort(UartCookie* uartCookie) { /* Save option settings */ if (tcsetattr(fd, TCSANOW, &options) != 0) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::configureUartPort: Failed to set options with error " << errno << ": " << strerror(errno); +#endif return fd; } return fd; @@ -152,7 +164,9 @@ void UartComIF::setDatasizeOptions(struct termios* options, UartCookie* uartCook options->c_cflag |= CS8; break; default: +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::setDatasizeOptions: Invalid size specified" << std::endl; +#endif break; } } @@ -259,7 +273,9 @@ void UartComIF::configureBaudrate(struct termios* options, UartCookie* uartCooki cfsetospeed(options, B460800); break; default: +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::configureBaudrate: Baudrate not supported" << std::endl; +#endif break; } } @@ -275,29 +291,37 @@ ReturnValue_t UartComIF::sendMessage(CookieIF *cookie, } if(sendData == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::sendMessage: Send data is nullptr" << std::endl; +#endif return RETURN_FAILED; } UartCookie* uartCookie = dynamic_cast(cookie); if(uartCookie == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::sendMessasge: Invalid UART Cookie!" << std::endl; +#endif return NULLPOINTER; } deviceFile = uartCookie->getDeviceFile(); uartDeviceMapIter = uartDeviceMap.find(deviceFile); if (uartDeviceMapIter == uartDeviceMap.end()) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::debug << "UartComIF::sendMessage: Device file " << deviceFile << "not in UART map" << std::endl; +#endif return RETURN_FAILED; } fd = uartDeviceMapIter->second.fileDescriptor; - if (write(fd, sendData, sendLen) != (int)sendLen) { + if (write(fd, sendData, sendLen) != static_cast(sendLen)) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::error << "UartComIF::sendMessage: Failed to send data with error code " << errno << ": Error description: " << strerror(errno) << std::endl; +#endif return RETURN_FAILED; } @@ -314,7 +338,9 @@ ReturnValue_t UartComIF::requestReceiveMessage(CookieIF *cookie, size_t requestL UartCookie* uartCookie = dynamic_cast(cookie); if(uartCookie == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::debug << "UartComIF::requestReceiveMessage: Invalid Uart Cookie!" << std::endl; +#endif return NULLPOINTER; } @@ -327,8 +353,10 @@ ReturnValue_t UartComIF::requestReceiveMessage(CookieIF *cookie, size_t requestL } if (uartDeviceMapIter == uartDeviceMap.end()) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::debug << "UartComIF::requestReceiveMessage: Device file " << deviceFile << " not in uart map" << std::endl; +#endif return RETURN_FAILED; } @@ -425,8 +453,10 @@ ReturnValue_t UartComIF::handleNoncanonicalRead(UartCookie &uartCookie, UartDevi } else if (bytesRead != static_cast(requestLen)) { if(uartCookie.isReplySizeFixed()) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::requestReceiveMessage: Only read " << bytesRead << " of " << requestLen << " bytes" << std::endl; +#endif return RETURN_FAILED; } } @@ -442,15 +472,19 @@ ReturnValue_t UartComIF::readReceivedMessage(CookieIF *cookie, UartCookie* uartCookie = dynamic_cast(cookie); if(uartCookie == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::debug << "UartComIF::readReceivedMessage: Invalid uart cookie!" << std::endl; +#endif return NULLPOINTER; } deviceFile = uartCookie->getDeviceFile(); uartDeviceMapIter = uartDeviceMap.find(deviceFile); if (uartDeviceMapIter == uartDeviceMap.end()) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::debug << "UartComIF::readReceivedMessage: Device file " << deviceFile << " not in uart map" << std::endl; +#endif return RETURN_FAILED; } @@ -468,7 +502,9 @@ ReturnValue_t UartComIF::flushUartRxBuffer(CookieIF *cookie) { UartDeviceMapIter uartDeviceMapIter; UartCookie* uartCookie = dynamic_cast(cookie); if(uartCookie == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::flushUartRxBuffer: Invalid uart cookie!" << std::endl; +#endif return NULLPOINTER; } deviceFile = uartCookie->getDeviceFile(); @@ -486,7 +522,9 @@ ReturnValue_t UartComIF::flushUartTxBuffer(CookieIF *cookie) { UartDeviceMapIter uartDeviceMapIter; UartCookie* uartCookie = dynamic_cast(cookie); if(uartCookie == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::flushUartTxBuffer: Invalid uart cookie!" << std::endl; +#endif return NULLPOINTER; } deviceFile = uartCookie->getDeviceFile(); @@ -504,7 +542,9 @@ ReturnValue_t UartComIF::flushUartTxAndRxBuf(CookieIF *cookie) { UartDeviceMapIter uartDeviceMapIter; UartCookie* uartCookie = dynamic_cast(cookie); if(uartCookie == nullptr) { +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "UartComIF::flushUartTxAndRxBuf: Invalid uart cookie!" << std::endl; +#endif return NULLPOINTER; } deviceFile = uartCookie->getDeviceFile(); diff --git a/hal/src/fsfw_hal/linux/uart/UartCookie.cpp b/hal/src/fsfw_hal/linux/uart/UartCookie.cpp index 1c52e9cd..140b1992 100644 --- a/hal/src/fsfw_hal/linux/uart/UartCookie.cpp +++ b/hal/src/fsfw_hal/linux/uart/UartCookie.cpp @@ -1,6 +1,6 @@ #include "fsfw_hal/linux/uart/UartCookie.h" -#include +#include UartCookie::UartCookie(object_id_t handlerId, std::string deviceFile, UartModes uartMode, uint32_t baudrate, size_t maxReplyLen): @@ -42,7 +42,9 @@ void UartCookie::setBitsPerWord(uint8_t bitsPerWord_) { case 8: break; default: +#if FSFW_CPP_OSTREAM_ENABLED == 1 sif::debug << "UartCookie::setBitsPerWord: Invalid bits per word specified" << std::endl; +#endif return; } bitsPerWord = bitsPerWord_; diff --git a/scripts/helper.py b/scripts/helper.py index 0bb430ee..1234a943 100755 --- a/scripts/helper.py +++ b/scripts/helper.py @@ -132,7 +132,9 @@ def handle_tests_type(args, build_dir_list: list): def create_tests_build_cfg(): os.mkdir(UNITTEST_FOLDER_NAME) os.chdir(UNITTEST_FOLDER_NAME) - os.system('cmake -DFSFW_OSAL=host -DFSFW_BUILD_UNITTESTS=ON ..') + cmake_cmd = "cmake -DFSFW_OSAL=host -DFSFW_BUILD_UNITTESTS=ON .." + print(f"Executing CMake command {cmake_cmd}") + os.system(cmake_cmd) os.chdir('..') diff --git a/src/fsfw/FSFW.h.in b/src/fsfw/FSFW.h.in index 7e52b646..a4982369 100644 --- a/src/fsfw/FSFW.h.in +++ b/src/fsfw/FSFW.h.in @@ -18,6 +18,10 @@ // FSFW core defines +#ifndef FSFW_TCP_RECV_WIRETAPPING_ENABLED +#define FSFW_TCP_RECV_WIRETAPPING_ENABLED 0 +#endif + #ifndef FSFW_CPP_OSTREAM_ENABLED #define FSFW_CPP_OSTREAM_ENABLED 1 #endif /* FSFW_CPP_OSTREAM_ENABLED */ diff --git a/src/fsfw/container/SimpleRingBuffer.h b/src/fsfw/container/SimpleRingBuffer.h index 6f31c5fb..dc0aeba8 100644 --- a/src/fsfw/container/SimpleRingBuffer.h +++ b/src/fsfw/container/SimpleRingBuffer.h @@ -5,8 +5,7 @@ #include /** - * @brief Circular buffer implementation, useful for buffering - * into data streams. + * @brief Circular buffer implementation, useful for buffering into data streams. * @details * Note that the deleteData() has to be called to increment the read pointer. * This class allocated dynamically, so @@ -20,8 +19,8 @@ public: * @param size * @param overwriteOld If the ring buffer is overflowing at a write * operation, the oldest data will be overwritten. - * @param maxExcessBytes These additional bytes will be allocated in addtion - * to the specified size to accomodate contiguous write operations + * @param maxExcessBytes These additional bytes will be allocated in addition + * to the specified size to accommodate continuous write operations * with getFreeElement. * */ @@ -32,10 +31,10 @@ public: * @param buffer * @param size * @param overwriteOld - * If the ring buffer is overflowing at a write operartion, the oldest data + * If the ring buffer is overflowing at a write operation, the oldest data * will be overwritten. * @param maxExcessBytes - * If the buffer can accomodate additional bytes for contigous write + * If the buffer can accommodate additional bytes for contiguous write * operations with getFreeElement, this is the maximum allowed additional * size */ @@ -48,7 +47,7 @@ public: * Write to circular buffer and increment write pointer by amount. * @param data * @param amount - * @return -@c RETURN_OK if write operation was successfull + * @return -@c RETURN_OK if write operation was successful * -@c RETURN_FAILED if */ ReturnValue_t writeData(const uint8_t* data, size_t amount); @@ -108,7 +107,7 @@ public: * Delete data by incrementing read pointer. * @param amount * @param deleteRemaining - * If the amount specified is larger than the remaing size to read and this + * If the amount specified is larger than the remaining size to read and this * is set to true, the remaining amount will be deleted as well * @param trueAmount [out] * If deleteRemaining was set to true, the amount deleted will be assigned diff --git a/src/fsfw/devicehandlers/AssemblyBase.h b/src/fsfw/devicehandlers/AssemblyBase.h index 6cac81b4..7d54f14d 100644 --- a/src/fsfw/devicehandlers/AssemblyBase.h +++ b/src/fsfw/devicehandlers/AssemblyBase.h @@ -7,7 +7,7 @@ /** * @brief Base class to implement reconfiguration and failure handling for - * redundant devices by monitoring their modes health states. + * redundant devices by monitoring their modes and health states. * @details * Documentation: Dissertation Baetz p.156, 157. * diff --git a/src/fsfw/devicehandlers/DeviceCommunicationIF.h b/src/fsfw/devicehandlers/DeviceCommunicationIF.h index e0b473d3..527e4700 100644 --- a/src/fsfw/devicehandlers/DeviceCommunicationIF.h +++ b/src/fsfw/devicehandlers/DeviceCommunicationIF.h @@ -85,9 +85,10 @@ public: * Called by DHB in the GET_WRITE doGetWrite(). * Get send confirmation that the data in sendMessage() was sent successfully. * @param cookie - * @return - @c RETURN_OK if data was sent successfull - * - Everything else triggers falure event with - * returnvalue as parameter 1 + * @return + * - @c RETURN_OK if data was sent successfully but a reply is expected + * - NO_REPLY_EXPECTED if data was sent successfully and no reply is expected + * - Everything else to indicate failure */ virtual ReturnValue_t getSendSuccess(CookieIF *cookie) = 0; diff --git a/src/fsfw/devicehandlers/DeviceHandlerBase.h b/src/fsfw/devicehandlers/DeviceHandlerBase.h index b182b611..4bc54ac4 100644 --- a/src/fsfw/devicehandlers/DeviceHandlerBase.h +++ b/src/fsfw/devicehandlers/DeviceHandlerBase.h @@ -334,8 +334,7 @@ protected: * - @c RETURN_OK to send command after #rawPacket and #rawPacketLen * have been set. * - @c HasActionsIF::EXECUTION_COMPLETE to generate a finish reply immediately. This can - * be used if no reply is expected. Otherwise, the developer can call #actionHelper.finish - * to finish the command handling. + * be used if no reply is expected * - Anything else triggers an event with the return code as a parameter as well as a * step reply failed with the return code */ diff --git a/src/fsfw/devicehandlers/DeviceHandlerIF.h b/src/fsfw/devicehandlers/DeviceHandlerIF.h index 1933c571..1fc57c42 100644 --- a/src/fsfw/devicehandlers/DeviceHandlerIF.h +++ b/src/fsfw/devicehandlers/DeviceHandlerIF.h @@ -120,7 +120,8 @@ public: static const ReturnValue_t WRONG_MODE_FOR_COMMAND = MAKE_RETURN_CODE(0xA5); static const ReturnValue_t TIMEOUT = MAKE_RETURN_CODE(0xA6); static const ReturnValue_t BUSY = MAKE_RETURN_CODE(0xA7); - static const ReturnValue_t NO_REPLY_EXPECTED = MAKE_RETURN_CODE(0xA8); //!< Used to indicate that this is a command-only command. + //!< Used to indicate that this is a command-only command. + static const ReturnValue_t NO_REPLY_EXPECTED = MAKE_RETURN_CODE(0xA8); static const ReturnValue_t NON_OP_TEMPERATURE = MAKE_RETURN_CODE(0xA9); static const ReturnValue_t COMMAND_NOT_IMPLEMENTED = MAKE_RETURN_CODE(0xAA); diff --git a/src/fsfw/osal/linux/CMakeLists.txt b/src/fsfw/osal/linux/CMakeLists.txt index 41800764..dcdade67 100644 --- a/src/fsfw/osal/linux/CMakeLists.txt +++ b/src/fsfw/osal/linux/CMakeLists.txt @@ -1,20 +1,19 @@ -target_sources(${LIB_FSFW_NAME} - PRIVATE - Clock.cpp - BinarySemaphore.cpp - CountingSemaphore.cpp - FixedTimeslotTask.cpp - InternalErrorCodes.cpp - MessageQueue.cpp - Mutex.cpp - MutexFactory.cpp - PeriodicPosixTask.cpp - PosixThread.cpp - QueueFactory.cpp - SemaphoreFactory.cpp - TaskFactory.cpp - tcpipHelpers.cpp - unixUtility.cpp +target_sources(${LIB_FSFW_NAME} PRIVATE + Clock.cpp + BinarySemaphore.cpp + CountingSemaphore.cpp + FixedTimeslotTask.cpp + InternalErrorCodes.cpp + MessageQueue.cpp + Mutex.cpp + MutexFactory.cpp + PeriodicPosixTask.cpp + PosixThread.cpp + QueueFactory.cpp + SemaphoreFactory.cpp + TaskFactory.cpp + tcpipHelpers.cpp + unixUtility.cpp ) find_package(Threads REQUIRED) diff --git a/src/fsfw/osal/linux/MessageQueue.cpp b/src/fsfw/osal/linux/MessageQueue.cpp index b068c04f..d028f9f7 100644 --- a/src/fsfw/osal/linux/MessageQueue.cpp +++ b/src/fsfw/osal/linux/MessageQueue.cpp @@ -285,10 +285,10 @@ ReturnValue_t MessageQueue::sendMessageFromMessageQueue(MessageQueueId_t sendTo, utility::printUnixErrorGeneric(CLASS_NAME, "sendMessageFromMessageQueue", "EBADF"); #if FSFW_CPP_OSTREAM_ENABLED == 1 - sif::warning << "mq_send to: " << sendTo << " sent from " - << sentFrom << "failed" << std::endl; + sif::warning << "mq_send to " << sendTo << " sent from " + << sentFrom << " failed" << std::endl; #else - sif::printWarning("mq_send to: %d sent from %d failed\n", sendTo, sentFrom); + sif::printWarning("mq_send to %d sent from %d failed\n", sendTo, sentFrom); #endif return DESTINATION_INVALID; } diff --git a/src/fsfw/subsystem/SubsystemBase.h b/src/fsfw/subsystem/SubsystemBase.h index 6b2e9b5f..a1de077d 100644 --- a/src/fsfw/subsystem/SubsystemBase.h +++ b/src/fsfw/subsystem/SubsystemBase.h @@ -62,7 +62,8 @@ protected: struct ChildInfo { MessageQueueId_t commandQueue; Mode_t mode; - Submode_t submode;bool healthChanged; + Submode_t submode; + bool healthChanged; }; Mode_t mode; diff --git a/tests/src/fsfw_tests/unit/CMakeLists.txt b/tests/src/fsfw_tests/unit/CMakeLists.txt index 164e3bde..7d5fbd43 100644 --- a/tests/src/fsfw_tests/unit/CMakeLists.txt +++ b/tests/src/fsfw_tests/unit/CMakeLists.txt @@ -21,3 +21,4 @@ add_subdirectory(storagemanager) add_subdirectory(globalfunctions) add_subdirectory(timemanager) add_subdirectory(tmtcpacket) +add_subdirectory(hal) diff --git a/tests/src/fsfw_tests/unit/hal/CMakeLists.txt b/tests/src/fsfw_tests/unit/hal/CMakeLists.txt new file mode 100644 index 00000000..ee14a3aa --- /dev/null +++ b/tests/src/fsfw_tests/unit/hal/CMakeLists.txt @@ -0,0 +1,3 @@ +target_sources(${FSFW_TEST_TGT} PRIVATE + testCommandExecutor.cpp +) diff --git a/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp b/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp new file mode 100644 index 00000000..0c825389 --- /dev/null +++ b/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp @@ -0,0 +1,110 @@ +#include + +#include "fsfw/serviceinterface.h" + +#include "fsfw/container/SimpleRingBuffer.h" +#include "fsfw/container/DynamicFIFO.h" + +#include "fsfw_hal/linux/CommandExecutor.h" +#include "fsfw/platform.h" + +#include +#include + +#ifdef PLATFORM_UNIX + +static const char TEST_FILE_NAME[] = "/tmp/fsfw-unittest-test.txt"; + +TEST_CASE( "Command Executor" , "[cmd-exec]") { + // Check blocking mode first + CommandExecutor cmdExecutor(1024); + std::string cmd = "echo \"test\" >> " + std::string(TEST_FILE_NAME); + REQUIRE(cmdExecutor.getCurrentState() == CommandExecutor::States::IDLE); + ReturnValue_t result = cmdExecutor.load(cmd, true, true); + REQUIRE(cmdExecutor.getCurrentState() == CommandExecutor::States::COMMAND_LOADED); + REQUIRE(result == HasReturnvaluesIF::RETURN_OK); + REQUIRE(cmdExecutor.execute() == HasReturnvaluesIF::RETURN_OK); + // Check that file exists with contents + std::ifstream file(TEST_FILE_NAME); + std::string line; + std::getline(file, line); + CHECK(line == "test"); + std::remove(TEST_FILE_NAME); + REQUIRE(cmdExecutor.getCurrentState() == CommandExecutor::States::IDLE); + + // Now check non-blocking mode + SimpleRingBuffer outputBuffer(524, true); + DynamicFIFO sizesFifo(12); + cmdExecutor.setRingBuffer(&outputBuffer, &sizesFifo); + + result = cmdExecutor.load("echo \"Hello World\"", false, false); + REQUIRE(result == HasReturnvaluesIF::RETURN_OK); + cmdExecutor.execute(); + bool bytesHaveBeenRead = false; + size_t limitIdx = 0; + while(result != CommandExecutor::EXECUTION_FINISHED) { + limitIdx++; + result = cmdExecutor.check(bytesHaveBeenRead); + REQUIRE(result != CommandExecutor::COMMAND_ERROR); + usleep(500); + REQUIRE(limitIdx < 5); + } + + limitIdx = 0; + CHECK(bytesHaveBeenRead == true); + CHECK(result == CommandExecutor::EXECUTION_FINISHED); + uint16_t readBytes = 0; + sizesFifo.retrieve(&readBytes); + REQUIRE(readBytes == 12); + REQUIRE(outputBuffer.getAvailableReadData() == 12); + uint8_t readBuffer[32]; + REQUIRE(outputBuffer.readData(readBuffer, 12) == HasReturnvaluesIF::RETURN_OK); + CHECK(strcmp(reinterpret_cast(readBuffer), "Hello World\n") == 0); + outputBuffer.deleteData(12, true); + // Test more complex command + result = cmdExecutor.load("ping -c 1 localhost", false, false); + REQUIRE(cmdExecutor.getCurrentState() == CommandExecutor::States::COMMAND_LOADED); + REQUIRE(cmdExecutor.execute() == HasReturnvaluesIF::RETURN_OK); + REQUIRE(cmdExecutor.getCurrentState() == CommandExecutor::States::PENDING); + limitIdx = 0; + while(result != CommandExecutor::EXECUTION_FINISHED) { + limitIdx++; + result = cmdExecutor.check(bytesHaveBeenRead); + REQUIRE(result != CommandExecutor::COMMAND_ERROR); + usleep(500); + REQUIRE(limitIdx < 20); + } + limitIdx = 0; + CHECK(bytesHaveBeenRead == true); + CHECK(result == CommandExecutor::EXECUTION_FINISHED); + REQUIRE(cmdExecutor.getCurrentState() == CommandExecutor::States::IDLE); + readBytes = 0; + sizesFifo.retrieve(&readBytes); + // That's about the size of the reply + bool beTrue = (readBytes > 200) and (readBytes < 300); + REQUIRE(beTrue); + uint8_t largerReadBuffer[1024] = {}; + outputBuffer.readData(largerReadBuffer, readBytes); + // You can also check this output in the debugger + std::string allTheReply(reinterpret_cast(largerReadBuffer)); + // I am just going to assume that this string is the same across ping implementations + // of different Linux systems + REQUIRE(allTheReply.find("localhost ping statistics") != std::string::npos); + + // Now check failing command + result = cmdExecutor.load("false", false, false); + REQUIRE(result == HasReturnvaluesIF::RETURN_OK); + result = cmdExecutor.execute(); + REQUIRE(result == HasReturnvaluesIF::RETURN_OK); + while(result != CommandExecutor::EXECUTION_FINISHED and result != HasReturnvaluesIF::RETURN_FAILED) { + limitIdx++; + result = cmdExecutor.check(bytesHaveBeenRead); + REQUIRE(result != CommandExecutor::COMMAND_ERROR); + usleep(500); + REQUIRE(limitIdx < 20); + } + REQUIRE(result == HasReturnvaluesIF::RETURN_FAILED); + REQUIRE(cmdExecutor.getLastError() == 1); +} + +#endif From 990e8672a888d6ac718f4edca953c1319766a06f Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 1 Feb 2022 13:47:16 +0100 Subject: [PATCH 2/7] update dockerfile --- automation/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/automation/Dockerfile b/automation/Dockerfile index 52e5acfb..a530a671 100644 --- a/automation/Dockerfile +++ b/automation/Dockerfile @@ -5,7 +5,7 @@ RUN apt-get --yes upgrade #tzdata is a dependency, won't install otherwise ARG DEBIAN_FRONTEND=noninteractive -RUN apt-get --yes install gcc g++ cmake make lcov git valgrind nano +RUN apt-get --yes install gcc g++ cmake make lcov git valgrind nano iputils-ping RUN git clone https://github.com/catchorg/Catch2.git && \ cd Catch2 && \ From d2b561ba2f068a80b0239b072fbb8c8919ea9535 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 1 Feb 2022 13:57:27 +0100 Subject: [PATCH 3/7] test --- tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp b/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp index 0c825389..959c3da2 100644 --- a/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp +++ b/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp @@ -59,7 +59,8 @@ TEST_CASE( "Command Executor" , "[cmd-exec]") { REQUIRE(outputBuffer.getAvailableReadData() == 12); uint8_t readBuffer[32]; REQUIRE(outputBuffer.readData(readBuffer, 12) == HasReturnvaluesIF::RETURN_OK); - CHECK(strcmp(reinterpret_cast(readBuffer), "Hello World\n") == 0); + std::string cmpString = "Hello World\n"; + CHECK(strcmp(reinterpret_cast(readBuffer), cmpString.c_str()) == 0); outputBuffer.deleteData(12, true); // Test more complex command result = cmdExecutor.load("ping -c 1 localhost", false, false); From 368481f88bdfb548e21645508a5f206f7777bfdc Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 1 Feb 2022 14:04:13 +0100 Subject: [PATCH 4/7] move strcmp outside of macro --- tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp b/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp index 959c3da2..090d60cb 100644 --- a/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp +++ b/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp @@ -60,7 +60,8 @@ TEST_CASE( "Command Executor" , "[cmd-exec]") { uint8_t readBuffer[32]; REQUIRE(outputBuffer.readData(readBuffer, 12) == HasReturnvaluesIF::RETURN_OK); std::string cmpString = "Hello World\n"; - CHECK(strcmp(reinterpret_cast(readBuffer), cmpString.c_str()) == 0); + int cmpResult = strcmp(reinterpret_cast(readBuffer), cmpString.c_str()); + CHECK(cmpResult == 0); outputBuffer.deleteData(12, true); // Test more complex command result = cmdExecutor.load("ping -c 1 localhost", false, false); From acbc2cd749c4ef1def66f27609c5aa5b544acfbe Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 1 Feb 2022 18:04:08 +0100 Subject: [PATCH 5/7] valgrind why --- tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp b/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp index 090d60cb..a72477f0 100644 --- a/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp +++ b/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp @@ -59,9 +59,11 @@ TEST_CASE( "Command Executor" , "[cmd-exec]") { REQUIRE(outputBuffer.getAvailableReadData() == 12); uint8_t readBuffer[32]; REQUIRE(outputBuffer.readData(readBuffer, 12) == HasReturnvaluesIF::RETURN_OK); + std::string readString(reinterpret_cast(readBuffer)); std::string cmpString = "Hello World\n"; - int cmpResult = strcmp(reinterpret_cast(readBuffer), cmpString.c_str()); - CHECK(cmpResult == 0); + //int cmpResult = strcmp(reinterpret_cast(readBuffer), cmpString.c_str()); + int cmpResult = (readString == cmpString); + CHECK(cmpResult); outputBuffer.deleteData(12, true); // Test more complex command result = cmdExecutor.load("ping -c 1 localhost", false, false); From fed39defd3b23b681d63d5d6405ca7d7f330510d Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Wed, 2 Feb 2022 09:56:12 +0100 Subject: [PATCH 6/7] update helper script --- scripts/helper.py | 93 ++++++++++++++++++++++++++++------------------- 1 file changed, 55 insertions(+), 38 deletions(-) diff --git a/scripts/helper.py b/scripts/helper.py index 1234a943..4e8a62b4 100755 --- a/scripts/helper.py +++ b/scripts/helper.py @@ -9,36 +9,44 @@ import webbrowser import shutil import sys import time +from shutil import which from typing import List -UNITTEST_FOLDER_NAME = 'build-tests' -DOCS_FOLDER_NAME = 'build-docs' +UNITTEST_FOLDER_NAME = "build-tests" +DOCS_FOLDER_NAME = "build-docs" def main(): parser = argparse.ArgumentParser(description="FSFW helper script") - choices = ('docs', 'tests') + choices = ("docs", "tests") parser.add_argument( - 'type', metavar='type', choices=choices, - help=f'Target type. Choices: {choices}' + "type", metavar="type", choices=choices, help=f"Target type. Choices: {choices}" ) parser.add_argument( - '-a', '--all', action='store_true', - help='Create, build and open specified type' + "-a", "--all", action="store_true", help="Create, build and open specified type" ) parser.add_argument( - '-c', '--create', action='store_true', - help='Create docs or test build configuration' + "-c", + "--create", + action="store_true", + help="Create docs or test build configuration", ) parser.add_argument( - '-b', '--build', action='store_true', - help='Build the specified type' + "-b", "--build", action="store_true", help="Build the specified type" ) parser.add_argument( - '-o', '--open', action='store_true', - help='Open test or documentation data in webbrowser' + "-o", + "--open", + action="store_true", + help="Open test or documentation data in webbrowser", + ) + parser.add_argument( + "-v", + "--valgrind", + action="store_true", + help="Run valgrind on generated test binary", ) args = parser.parse_args() @@ -46,26 +54,26 @@ def main(): args.build = True args.create = True args.open = True - elif not args.build and not args.create and not args.open: + elif not args.build and not args.create and not args.open and not args.valgrind: print( - 'Please select at least one operation to perform. ' - 'Use helper.py -h for more information' + "Please select at least one operation to perform. " + "Use helper.py -h for more information" ) sys.exit(1) # This script can be called from root and from script folder - if not os.path.isfile('README.md'): - os.chdir('..') + if not os.path.isfile("README.md"): + os.chdir("..") build_dir_list = [] if not args.create: build_dir_list = build_build_dir_list() - if args.type == 'tests': + if args.type == "tests": handle_tests_type(args, build_dir_list) - elif args.type == 'docs': + elif args.type == "docs": handle_docs_type(args, build_dir_list) else: - print('Invalid or unknown type') + print("Invalid or unknown type") sys.exit(1) @@ -76,7 +84,9 @@ def handle_docs_type(args, build_dir_list: list): create_docs_build_cfg() build_directory = DOCS_FOLDER_NAME elif len(build_dir_list) == 0: - print('No valid CMake docs build directory found. Trying to set up docs build system') + print( + "No valid CMake docs build directory found. Trying to set up docs build system" + ) shutil.rmtree(DOCS_FOLDER_NAME) create_docs_build_cfg() build_directory = DOCS_FOLDER_NAME @@ -87,18 +97,18 @@ def handle_docs_type(args, build_dir_list: list): build_directory = determine_build_dir(build_dir_list) os.chdir(build_directory) if args.build: - os.system('cmake --build . -j') + os.system("cmake --build . -j") if args.open: - if not os.path.isfile('docs/sphinx/index.html'): + if not os.path.isfile("docs/sphinx/index.html"): # try again.. - os.system('cmake --build . -j') - if not os.path.isfile('docs/sphinx/index.html'): + os.system("cmake --build . -j") + if not os.path.isfile("docs/sphinx/index.html"): print( "No Sphinx documentation file detected. " "Try to build it first with the -b argument" ) sys.exit(1) - webbrowser.open('docs/sphinx/index.html') + webbrowser.open("docs/sphinx/index.html") def handle_tests_type(args, build_dir_list: list): @@ -109,8 +119,8 @@ def handle_tests_type(args, build_dir_list: list): build_directory = UNITTEST_FOLDER_NAME elif len(build_dir_list) == 0: print( - 'No valid CMake tests build directory found. ' - 'Trying to set up test build system' + "No valid CMake tests build directory found. " + "Trying to set up test build system" ) create_tests_build_cfg() build_directory = UNITTEST_FOLDER_NAME @@ -123,26 +133,33 @@ def handle_tests_type(args, build_dir_list: list): if args.build: perform_lcov_operation(build_directory, False) if args.open: - if not os.path.isdir('fsfw-tests_coverage'): - print("No Unittest folder detected. Try to build them first with the -b argument") + if not os.path.isdir("fsfw-tests_coverage"): + print( + "No Unittest folder detected. Try to build them first with the -b argument" + ) sys.exit(1) - webbrowser.open('fsfw-tests_coverage/index.html') + webbrowser.open("fsfw-tests_coverage/index.html") + if args.valgrind: + if which("valgrind") is None: + print("Please install valgrind first") + sys.exit(1) + os.chdir(UNITTEST_FOLDER_NAME) + os.system("valgrind --leak-check=full ./fsfw-tests") + os.chdir("..") def create_tests_build_cfg(): os.mkdir(UNITTEST_FOLDER_NAME) os.chdir(UNITTEST_FOLDER_NAME) - cmake_cmd = "cmake -DFSFW_OSAL=host -DFSFW_BUILD_UNITTESTS=ON .." - print(f"Executing CMake command {cmake_cmd}") - os.system(cmake_cmd) - os.chdir('..') + os.system("cmake -DFSFW_OSAL=host -DFSFW_BUILD_UNITTESTS=ON ..") + os.chdir("..") def create_docs_build_cfg(): os.mkdir(DOCS_FOLDER_NAME) os.chdir(DOCS_FOLDER_NAME) - os.system('cmake -DFSFW_OSAL=host -DFSFW_BUILD_DOCS=ON ..') - os.chdir('..') + os.system("cmake -DFSFW_OSAL=host -DFSFW_BUILD_DOCS=ON ..") + os.chdir("..") def build_build_dir_list() -> list: From e0c50477cbc0dfb7f861cfb55444de6c5cf1d5df Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Wed, 2 Feb 2022 10:00:57 +0100 Subject: [PATCH 7/7] it actually was an uninitialized array --- tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp b/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp index a72477f0..d7e55af5 100644 --- a/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp +++ b/tests/src/fsfw_tests/unit/hal/testCommandExecutor.cpp @@ -57,13 +57,11 @@ TEST_CASE( "Command Executor" , "[cmd-exec]") { sizesFifo.retrieve(&readBytes); REQUIRE(readBytes == 12); REQUIRE(outputBuffer.getAvailableReadData() == 12); - uint8_t readBuffer[32]; + uint8_t readBuffer[32] = {}; REQUIRE(outputBuffer.readData(readBuffer, 12) == HasReturnvaluesIF::RETURN_OK); std::string readString(reinterpret_cast(readBuffer)); std::string cmpString = "Hello World\n"; - //int cmpResult = strcmp(reinterpret_cast(readBuffer), cmpString.c_str()); - int cmpResult = (readString == cmpString); - CHECK(cmpResult); + CHECK(readString == cmpString); outputBuffer.deleteData(12, true); // Test more complex command result = cmdExecutor.load("ping -c 1 localhost", false, false);