From e3c968096b3a34b29bb8896afe0d1d698f834c6f Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Sat, 18 Feb 2023 13:07:38 +0100 Subject: [PATCH 1/6] i2c small improvements --- src/fsfw_hal/linux/i2c/I2cComIF.cpp | 10 +++++----- src/fsfw_hal/linux/i2c/I2cComIF.h | 2 +- src/fsfw_hal/linux/i2c/I2cCookie.cpp | 4 ++-- src/fsfw_hal/linux/i2c/I2cCookie.h | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/fsfw_hal/linux/i2c/I2cComIF.cpp b/src/fsfw_hal/linux/i2c/I2cComIF.cpp index 66c2bb51..d66e91de 100644 --- a/src/fsfw_hal/linux/i2c/I2cComIF.cpp +++ b/src/fsfw_hal/linux/i2c/I2cComIF.cpp @@ -103,7 +103,7 @@ ReturnValue_t I2cComIF::sendMessage(CookieIF* cookie, const uint8_t* sendData, s if (fileHelper.getOpenResult() != returnvalue::OK) { return fileHelper.getOpenResult(); } - result = openDevice(deviceFile, i2cAddress, &fd); + result = openI2cSlave(deviceFile, i2cAddress, fd); if (result != returnvalue::OK) { return result; } @@ -162,7 +162,7 @@ ReturnValue_t I2cComIF::requestReceiveMessage(CookieIF* cookie, size_t requestLe if (fileHelper.getOpenResult() != returnvalue::OK) { return fileHelper.getOpenResult(); } - result = openDevice(deviceFile, i2cAddress, &fd); + result = openI2cSlave(deviceFile, i2cAddress, fd); if (result != returnvalue::OK) { return result; } @@ -220,9 +220,9 @@ ReturnValue_t I2cComIF::readReceivedMessage(CookieIF* cookie, uint8_t** buffer, return returnvalue::OK; } -ReturnValue_t I2cComIF::openDevice(std::string deviceFile, address_t i2cAddress, - int* fileDescriptor) { - if (ioctl(*fileDescriptor, I2C_SLAVE, i2cAddress) < 0) { +ReturnValue_t I2cComIF::openI2cSlave(const std::string& deviceFile, address_t i2cAddress, + int fileDescriptor) { + if (ioctl(fileDescriptor, I2C_SLAVE, i2cAddress) < 0) { #if FSFW_VERBOSE_LEVEL >= 1 #if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "I2cComIF: Specifying target device failed with error code " << errno << "." diff --git a/src/fsfw_hal/linux/i2c/I2cComIF.h b/src/fsfw_hal/linux/i2c/I2cComIF.h index 8c44cee0..4bcc8cfa 100644 --- a/src/fsfw_hal/linux/i2c/I2cComIF.h +++ b/src/fsfw_hal/linux/i2c/I2cComIF.h @@ -49,7 +49,7 @@ class I2cComIF : public DeviceCommunicationIF, public SystemObject { * @param fileDescriptor Pointer to device descriptor. * @return returnvalue::OK if successful, otherwise returnvalue::FAILED. */ - ReturnValue_t openDevice(std::string deviceFile, address_t i2cAddress, int *fileDescriptor); + ReturnValue_t openI2cSlave(const std::string& deviceFile, address_t i2cAddress, int fileDescriptor); }; #endif /* LINUX_I2C_I2COMIF_H_ */ diff --git a/src/fsfw_hal/linux/i2c/I2cCookie.cpp b/src/fsfw_hal/linux/i2c/I2cCookie.cpp index 0186be60..f41527a3 100644 --- a/src/fsfw_hal/linux/i2c/I2cCookie.cpp +++ b/src/fsfw_hal/linux/i2c/I2cCookie.cpp @@ -1,12 +1,12 @@ #include "fsfw_hal/linux/i2c/I2cCookie.h" I2cCookie::I2cCookie(address_t i2cAddress_, size_t maxReplyLen_, std::string deviceFile_) - : i2cAddress(i2cAddress_), maxReplyLen(maxReplyLen_), deviceFile(deviceFile_) {} + : i2cAddress(i2cAddress_), maxReplyLen(maxReplyLen_), deviceFile(std::move(deviceFile_)) {} address_t I2cCookie::getAddress() const { return i2cAddress; } size_t I2cCookie::getMaxReplyLen() const { return maxReplyLen; } -std::string I2cCookie::getDeviceFile() const { return deviceFile; } +const std::string& I2cCookie::getDeviceFile() const { return deviceFile; } I2cCookie::~I2cCookie() {} diff --git a/src/fsfw_hal/linux/i2c/I2cCookie.h b/src/fsfw_hal/linux/i2c/I2cCookie.h index 8be71205..e54bcd55 100644 --- a/src/fsfw_hal/linux/i2c/I2cCookie.h +++ b/src/fsfw_hal/linux/i2c/I2cCookie.h @@ -25,7 +25,7 @@ class I2cCookie : public CookieIF { address_t getAddress() const; size_t getMaxReplyLen() const; - std::string getDeviceFile() const; + const std::string& getDeviceFile() const; uint8_t errorCounter = 0; From fcd84b59aee192fb0eb3a3550f036af952a68fc1 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Sat, 18 Feb 2023 13:37:39 +0100 Subject: [PATCH 2/6] how the hell does this break anything --- src/fsfw_hal/linux/i2c/I2cComIF.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/fsfw_hal/linux/i2c/I2cComIF.cpp b/src/fsfw_hal/linux/i2c/I2cComIF.cpp index d66e91de..10e493e5 100644 --- a/src/fsfw_hal/linux/i2c/I2cComIF.cpp +++ b/src/fsfw_hal/linux/i2c/I2cComIF.cpp @@ -66,8 +66,7 @@ ReturnValue_t I2cComIF::initializeInterface(CookieIF* cookie) { ReturnValue_t I2cComIF::sendMessage(CookieIF* cookie, const uint8_t* sendData, size_t sendLen) { ReturnValue_t result; - int fd; - std::string deviceFile; + int fd = 0; if (sendData == nullptr) { #if FSFW_CPP_OSTREAM_ENABLED == 1 @@ -98,7 +97,7 @@ ReturnValue_t I2cComIF::sendMessage(CookieIF* cookie, const uint8_t* sendData, s return returnvalue::FAILED; } - deviceFile = i2cCookie->getDeviceFile(); + const auto& deviceFile = i2cCookie->getDeviceFile(); UnixFileGuard fileHelper(deviceFile, &fd, O_RDWR, "I2cComIF::sendMessage"); if (fileHelper.getOpenResult() != returnvalue::OK) { return fileHelper.getOpenResult(); @@ -132,7 +131,6 @@ ReturnValue_t I2cComIF::getSendSuccess(CookieIF* cookie) { return returnvalue::O ReturnValue_t I2cComIF::requestReceiveMessage(CookieIF* cookie, size_t requestLen) { ReturnValue_t result; int fd; - std::string deviceFile; if (requestLen == 0) { return returnvalue::OK; @@ -157,7 +155,7 @@ ReturnValue_t I2cComIF::requestReceiveMessage(CookieIF* cookie, size_t requestLe } i2cDeviceMapIter->second.replyLen = 0; - deviceFile = i2cCookie->getDeviceFile(); + auto& deviceFile = i2cCookie->getDeviceFile(); UnixFileGuard fileHelper(deviceFile, &fd, O_RDWR, "I2cComIF::requestReceiveMessage"); if (fileHelper.getOpenResult() != returnvalue::OK) { return fileHelper.getOpenResult(); From 3568bdbecf74733970211d7ff713ad030b332a98 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Sat, 18 Feb 2023 13:45:49 +0100 Subject: [PATCH 3/6] lets see if this fixes the issue --- src/fsfw_hal/linux/UnixFileGuard.cpp | 15 ++++----------- src/fsfw_hal/linux/UnixFileGuard.h | 12 ++++++++++-- src/fsfw_hal/linux/i2c/I2cComIF.cpp | 16 +++++++++------- src/fsfw_hal/linux/i2c/I2cComIF.h | 3 ++- src/fsfw_hal/linux/spi/SpiComIF.cpp | 6 +++--- 5 files changed, 28 insertions(+), 24 deletions(-) diff --git a/src/fsfw_hal/linux/UnixFileGuard.cpp b/src/fsfw_hal/linux/UnixFileGuard.cpp index 3e916ba2..fb8493ca 100644 --- a/src/fsfw_hal/linux/UnixFileGuard.cpp +++ b/src/fsfw_hal/linux/UnixFileGuard.cpp @@ -6,14 +6,11 @@ #include "fsfw/FSFW.h" #include "fsfw/serviceinterface.h" -UnixFileGuard::UnixFileGuard(const std::string& device, int* fileDescriptor, int flags, +UnixFileGuard::UnixFileGuard(const std::string& device, int& fileDescriptor, int flags, std::string diagnosticPrefix) : fileDescriptor(fileDescriptor) { - if (fileDescriptor == nullptr) { - return; - } - *fileDescriptor = open(device.c_str(), flags); - if (*fileDescriptor < 0) { + fileDescriptor = open(device.c_str(), flags); + if (fileDescriptor < 0) { #if FSFW_VERBOSE_LEVEL >= 1 #if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << diagnosticPrefix << ": Opening device failed with error code " << errno << ": " @@ -27,10 +24,6 @@ UnixFileGuard::UnixFileGuard(const std::string& device, int* fileDescriptor, int } } -UnixFileGuard::~UnixFileGuard() { - if (fileDescriptor != nullptr) { - close(*fileDescriptor); - } -} +UnixFileGuard::~UnixFileGuard() { close(fileDescriptor); } ReturnValue_t UnixFileGuard::getOpenResult() const { return openStatus; } diff --git a/src/fsfw_hal/linux/UnixFileGuard.h b/src/fsfw_hal/linux/UnixFileGuard.h index eec85233..19da5f4a 100644 --- a/src/fsfw_hal/linux/UnixFileGuard.h +++ b/src/fsfw_hal/linux/UnixFileGuard.h @@ -15,7 +15,15 @@ class UnixFileGuard { static constexpr ReturnValue_t OPEN_FILE_FAILED = 1; - UnixFileGuard(const std::string& device, int* fileDescriptor, int flags, + /** + * Open a device and assign the given file descriptor variable + * @param device [in] Device name. + * @param fileDescriptor [in/out] Will be assigned by file guard and re-used to + * close the guard. + * @param flags + * @param diagnosticPrefix + */ + UnixFileGuard(const std::string& device, int& fileDescriptor, int flags, std::string diagnosticPrefix = ""); virtual ~UnixFileGuard(); @@ -23,7 +31,7 @@ class UnixFileGuard { ReturnValue_t getOpenResult() const; private: - int* fileDescriptor = nullptr; + int fileDescriptor = 0; ReturnValue_t openStatus = returnvalue::OK; }; diff --git a/src/fsfw_hal/linux/i2c/I2cComIF.cpp b/src/fsfw_hal/linux/i2c/I2cComIF.cpp index 10e493e5..0a88edc7 100644 --- a/src/fsfw_hal/linux/i2c/I2cComIF.cpp +++ b/src/fsfw_hal/linux/i2c/I2cComIF.cpp @@ -98,16 +98,17 @@ ReturnValue_t I2cComIF::sendMessage(CookieIF* cookie, const uint8_t* sendData, s } const auto& deviceFile = i2cCookie->getDeviceFile(); - UnixFileGuard fileHelper(deviceFile, &fd, O_RDWR, "I2cComIF::sendMessage"); + UnixFileGuard fileHelper(deviceFile, fd, O_RDWR, "I2cComIF::sendMessage"); if (fileHelper.getOpenResult() != returnvalue::OK) { return fileHelper.getOpenResult(); } - result = openI2cSlave(deviceFile, i2cAddress, fd); + int slaveFd = 0; + result = openI2cSlave(deviceFile, i2cAddress, slaveFd); if (result != returnvalue::OK) { return result; } - if (write(fd, sendData, sendLen) != static_cast(sendLen)) { + if (write(slaveFd, sendData, sendLen) != static_cast(sendLen)) { i2cCookie->errorCounter++; if (i2cCookie->errorCounter < 3) { #if FSFW_CPP_OSTREAM_ENABLED == 1 @@ -130,7 +131,7 @@ ReturnValue_t I2cComIF::getSendSuccess(CookieIF* cookie) { return returnvalue::O ReturnValue_t I2cComIF::requestReceiveMessage(CookieIF* cookie, size_t requestLen) { ReturnValue_t result; - int fd; + int fd = 0; if (requestLen == 0) { return returnvalue::OK; @@ -156,11 +157,12 @@ ReturnValue_t I2cComIF::requestReceiveMessage(CookieIF* cookie, size_t requestLe i2cDeviceMapIter->second.replyLen = 0; auto& deviceFile = i2cCookie->getDeviceFile(); - UnixFileGuard fileHelper(deviceFile, &fd, O_RDWR, "I2cComIF::requestReceiveMessage"); + UnixFileGuard fileHelper(deviceFile, fd, O_RDWR, "I2cComIF::requestReceiveMessage"); if (fileHelper.getOpenResult() != returnvalue::OK) { return fileHelper.getOpenResult(); } - result = openI2cSlave(deviceFile, i2cAddress, fd); + int slaveFd = 0; + result = openI2cSlave(deviceFile, i2cAddress, slaveFd); if (result != returnvalue::OK) { return result; } @@ -219,7 +221,7 @@ ReturnValue_t I2cComIF::readReceivedMessage(CookieIF* cookie, uint8_t** buffer, } ReturnValue_t I2cComIF::openI2cSlave(const std::string& deviceFile, address_t i2cAddress, - int fileDescriptor) { + int& fileDescriptor) { if (ioctl(fileDescriptor, I2C_SLAVE, i2cAddress) < 0) { #if FSFW_VERBOSE_LEVEL >= 1 #if FSFW_CPP_OSTREAM_ENABLED == 1 diff --git a/src/fsfw_hal/linux/i2c/I2cComIF.h b/src/fsfw_hal/linux/i2c/I2cComIF.h index 4bcc8cfa..983ce734 100644 --- a/src/fsfw_hal/linux/i2c/I2cComIF.h +++ b/src/fsfw_hal/linux/i2c/I2cComIF.h @@ -49,7 +49,8 @@ class I2cComIF : public DeviceCommunicationIF, public SystemObject { * @param fileDescriptor Pointer to device descriptor. * @return returnvalue::OK if successful, otherwise returnvalue::FAILED. */ - ReturnValue_t openI2cSlave(const std::string& deviceFile, address_t i2cAddress, int fileDescriptor); + ReturnValue_t openI2cSlave(const std::string &deviceFile, address_t i2cAddress, + int &fileDescriptor); }; #endif /* LINUX_I2C_I2COMIF_H_ */ diff --git a/src/fsfw_hal/linux/spi/SpiComIF.cpp b/src/fsfw_hal/linux/spi/SpiComIF.cpp index ea25e837..d285b120 100644 --- a/src/fsfw_hal/linux/spi/SpiComIF.cpp +++ b/src/fsfw_hal/linux/spi/SpiComIF.cpp @@ -75,7 +75,7 @@ ReturnValue_t SpiComIF::initializeInterface(CookieIF* cookie) { spiCookie->getSpiParameters(spiMode, spiSpeed, ¶ms); int fileDescriptor = 0; - UnixFileGuard fileHelper(dev, &fileDescriptor, O_RDWR, "SpiComIF::initializeInterface"); + UnixFileGuard fileHelper(dev, fileDescriptor, O_RDWR, "SpiComIF::initializeInterface"); if (fileHelper.getOpenResult() != returnvalue::OK) { return fileHelper.getOpenResult(); } @@ -171,7 +171,7 @@ ReturnValue_t SpiComIF::performRegularSendOperation(SpiCookie* spiCookie, const int retval = 0; /* Prepare transfer */ int fileDescriptor = 0; - UnixFileGuard fileHelper(dev, &fileDescriptor, O_RDWR, "SpiComIF::sendMessage"); + UnixFileGuard fileHelper(dev, fileDescriptor, O_RDWR, "SpiComIF::sendMessage"); if (fileHelper.getOpenResult() != returnvalue::OK) { return OPENING_FILE_FAILED; } @@ -285,7 +285,7 @@ ReturnValue_t SpiComIF::requestReceiveMessage(CookieIF* cookie, size_t requestLe ReturnValue_t SpiComIF::performHalfDuplexReception(SpiCookie* spiCookie) { ReturnValue_t result = returnvalue::OK; int fileDescriptor = 0; - UnixFileGuard fileHelper(dev, &fileDescriptor, O_RDWR, "SpiComIF::requestReceiveMessage"); + UnixFileGuard fileHelper(dev, fileDescriptor, O_RDWR, "SpiComIF::requestReceiveMessage"); if (fileHelper.getOpenResult() != returnvalue::OK) { return OPENING_FILE_FAILED; } From 5f9eb01d943d8eed610d395aa3f0c6017ea1c58f Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Sat, 18 Feb 2023 13:46:51 +0100 Subject: [PATCH 4/6] better naming --- src/fsfw_hal/linux/UnixFileGuard.cpp | 4 ++-- src/fsfw_hal/linux/UnixFileGuard.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/fsfw_hal/linux/UnixFileGuard.cpp b/src/fsfw_hal/linux/UnixFileGuard.cpp index fb8493ca..0a44e445 100644 --- a/src/fsfw_hal/linux/UnixFileGuard.cpp +++ b/src/fsfw_hal/linux/UnixFileGuard.cpp @@ -8,7 +8,7 @@ UnixFileGuard::UnixFileGuard(const std::string& device, int& fileDescriptor, int flags, std::string diagnosticPrefix) - : fileDescriptor(fileDescriptor) { + : cachedFd(fileDescriptor) { fileDescriptor = open(device.c_str(), flags); if (fileDescriptor < 0) { #if FSFW_VERBOSE_LEVEL >= 1 @@ -24,6 +24,6 @@ UnixFileGuard::UnixFileGuard(const std::string& device, int& fileDescriptor, int } } -UnixFileGuard::~UnixFileGuard() { close(fileDescriptor); } +UnixFileGuard::~UnixFileGuard() { close(cachedFd); } ReturnValue_t UnixFileGuard::getOpenResult() const { return openStatus; } diff --git a/src/fsfw_hal/linux/UnixFileGuard.h b/src/fsfw_hal/linux/UnixFileGuard.h index 19da5f4a..25386aa3 100644 --- a/src/fsfw_hal/linux/UnixFileGuard.h +++ b/src/fsfw_hal/linux/UnixFileGuard.h @@ -31,7 +31,7 @@ class UnixFileGuard { ReturnValue_t getOpenResult() const; private: - int fileDescriptor = 0; + int cachedFd = 0; ReturnValue_t openStatus = returnvalue::OK; }; From 2d6622b8b86f9e77eb9e350698cfb10d863f32f7 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Sat, 18 Feb 2023 13:51:24 +0100 Subject: [PATCH 5/6] wtf is this interface --- src/fsfw_hal/linux/i2c/I2cComIF.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/fsfw_hal/linux/i2c/I2cComIF.cpp b/src/fsfw_hal/linux/i2c/I2cComIF.cpp index 0a88edc7..b1d54701 100644 --- a/src/fsfw_hal/linux/i2c/I2cComIF.cpp +++ b/src/fsfw_hal/linux/i2c/I2cComIF.cpp @@ -102,13 +102,12 @@ ReturnValue_t I2cComIF::sendMessage(CookieIF* cookie, const uint8_t* sendData, s if (fileHelper.getOpenResult() != returnvalue::OK) { return fileHelper.getOpenResult(); } - int slaveFd = 0; - result = openI2cSlave(deviceFile, i2cAddress, slaveFd); + result = openI2cSlave(deviceFile, i2cAddress, fd); if (result != returnvalue::OK) { return result; } - if (write(slaveFd, sendData, sendLen) != static_cast(sendLen)) { + if (write(fd, sendData, sendLen) != static_cast(sendLen)) { i2cCookie->errorCounter++; if (i2cCookie->errorCounter < 3) { #if FSFW_CPP_OSTREAM_ENABLED == 1 @@ -161,8 +160,7 @@ ReturnValue_t I2cComIF::requestReceiveMessage(CookieIF* cookie, size_t requestLe if (fileHelper.getOpenResult() != returnvalue::OK) { return fileHelper.getOpenResult(); } - int slaveFd = 0; - result = openI2cSlave(deviceFile, i2cAddress, slaveFd); + result = openI2cSlave(deviceFile, i2cAddress, fd); if (result != returnvalue::OK) { return result; } From c8469ca6473f64676e007e2e2f1c733fe6252053 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Sat, 18 Feb 2023 14:03:19 +0100 Subject: [PATCH 6/6] a lot of bug potential here --- src/fsfw_hal/linux/UnixFileGuard.cpp | 4 ++-- src/fsfw_hal/linux/UnixFileGuard.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/fsfw_hal/linux/UnixFileGuard.cpp b/src/fsfw_hal/linux/UnixFileGuard.cpp index 0a44e445..3178f39d 100644 --- a/src/fsfw_hal/linux/UnixFileGuard.cpp +++ b/src/fsfw_hal/linux/UnixFileGuard.cpp @@ -8,7 +8,7 @@ UnixFileGuard::UnixFileGuard(const std::string& device, int& fileDescriptor, int flags, std::string diagnosticPrefix) - : cachedFd(fileDescriptor) { + : fdRef(fileDescriptor) { fileDescriptor = open(device.c_str(), flags); if (fileDescriptor < 0) { #if FSFW_VERBOSE_LEVEL >= 1 @@ -24,6 +24,6 @@ UnixFileGuard::UnixFileGuard(const std::string& device, int& fileDescriptor, int } } -UnixFileGuard::~UnixFileGuard() { close(cachedFd); } +UnixFileGuard::~UnixFileGuard() { close(fdRef); } ReturnValue_t UnixFileGuard::getOpenResult() const { return openStatus; } diff --git a/src/fsfw_hal/linux/UnixFileGuard.h b/src/fsfw_hal/linux/UnixFileGuard.h index 25386aa3..884e551c 100644 --- a/src/fsfw_hal/linux/UnixFileGuard.h +++ b/src/fsfw_hal/linux/UnixFileGuard.h @@ -31,7 +31,7 @@ class UnixFileGuard { ReturnValue_t getOpenResult() const; private: - int cachedFd = 0; + int& fdRef; ReturnValue_t openStatus = returnvalue::OK; };