CHANGELOG.md

@@ -60,8 +60,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## Additions
 
-- Linux HAL: Getter functions for SPI speed and mode.
-- Linux HAL: Add wiretapping option for I2C. Enabled with `FSFW_HAL_I2C_WIRETAPPING` defined to 1
 - Dedicated Version class and constant `fsfw::FSFW_VERSION` containing version information
   inside `fsfw/version.h`
   PR: https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/559
@@ -69,6 +67,17 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   PR: https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/592
 - `Subsystem`: New API to add table and sequence entries
 
+## HAL
+
+- SPI: Cache the SPI device in the communication interface. Architecturally, this makes a
+  lot more sense because each ComIF should be responsible for one SPI bus.
+- SPI: Move the empty transfer to update the line polarity to separate function. This means
+  it is not automatically called when calling the setter function for SPI speed and mode.
+  The user should call this function after locking the CS mutex if multiple SPI devices with
+  differing speeds and modes are attached to one bus.
+- SPI: Getter functions for SPI speed and mode.
+- I2C: Add wiretapping option for I2C. Enabled with `FSFW_HAL_I2C_WIRETAPPING` defined to 1.
+
 ## Fixed
 
 - TCP TMTC Server: `MutexGuard` was not created properly in

hal/src/fsfw_hal/linux/UnixFileGuard.cpp

@@ -6,7 +6,7 @@
 #include "fsfw/FSFW.h"
 #include "fsfw/serviceinterface.h"
 
-UnixFileGuard::UnixFileGuard(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) {

hal/src/fsfw_hal/linux/UnixFileGuard.h

@@ -15,7 +15,7 @@ class UnixFileGuard {
 
   static constexpr ReturnValue_t OPEN_FILE_FAILED = 1;
 
-  UnixFileGuard(std::string device, int* fileDescriptor, int flags,
+  UnixFileGuard(const std::string& device, int* fileDescriptor, int flags,
                 std::string diagnosticPrefix = "");
 
   virtual ~UnixFileGuard();

hal/src/fsfw_hal/linux/spi/SpiComIF.cpp

@@ -15,8 +15,8 @@
 #include "fsfw_hal/linux/spi/SpiCookie.h"
 #include "fsfw_hal/linux/utility.h"
 
-SpiComIF::SpiComIF(object_id_t objectId, GpioIF* gpioComIF)
+SpiComIF::SpiComIF(object_id_t objectId, std::string devname, GpioIF* gpioComIF)
-    : SystemObject(objectId), gpioComIF(gpioComIF) {
+    : SystemObject(objectId), gpioComIF(gpioComIF), dev(std::move(devname)) {
   if (gpioComIF == nullptr) {
 #if FSFW_VERBOSE_LEVEL >= 1
 #if FSFW_CPP_OSTREAM_ENABLED == 1
@@ -27,7 +27,7 @@ SpiComIF::SpiComIF(object_id_t objectId, GpioIF* gpioComIF)
 #endif /* FSFW_VERBOSE_LEVEL >= 1 */
   }
 
-  spiMutex = MutexFactory::instance()->createMutex();
+  csMutex = MutexFactory::instance()->createMutex();
 }
 
 ReturnValue_t SpiComIF::initializeInterface(CookieIF* cookie) {
@@ -85,8 +85,7 @@ ReturnValue_t SpiComIF::initializeInterface(CookieIF* cookie) {
   spiCookie->getSpiParameters(spiMode, spiSpeed, &params);
 
   int fileDescriptor = 0;
-  UnixFileGuard fileHelper(spiCookie->getSpiDevice(), &fileDescriptor, O_RDWR,
+  UnixFileGuard fileHelper(dev, &fileDescriptor, O_RDWR, "SpiComIF::initializeInterface");
-                           "SpiComIF::initializeInterface");
   if (fileHelper.getOpenResult() != HasReturnvaluesIF::RETURN_OK) {
     return fileHelper.getOpenResult();
   }
@@ -182,8 +181,7 @@ ReturnValue_t SpiComIF::performRegularSendOperation(SpiCookie* spiCookie, const
   int retval = 0;
   /* Prepare transfer */
   int fileDescriptor = 0;
-  std::string device = spiCookie->getSpiDevice();
+  UnixFileGuard fileHelper(dev, &fileDescriptor, O_RDWR, "SpiComIF::sendMessage");
-  UnixFileGuard fileHelper(device, &fileDescriptor, O_RDWR, "SpiComIF::sendMessage");
   if (fileHelper.getOpenResult() != HasReturnvaluesIF::RETURN_OK) {
     return OPENING_FILE_FAILED;
   }
@@ -199,7 +197,7 @@ ReturnValue_t SpiComIF::performRegularSendOperation(SpiCookie* spiCookie, const
 
   /* Pull SPI CS low. For now, no support for active high given  */
   if (gpioId != gpio::NO_GPIO) {
-    result = spiMutex->lockMutex(timeoutType, timeoutMs);
+    result = csMutex->lockMutex(timeoutType, timeoutMs);
     if (result != RETURN_OK) {
 #if FSFW_VERBOSE_LEVEL >= 1
 #if FSFW_CPP_OSTREAM_ENABLED == 1
@@ -210,6 +208,7 @@ ReturnValue_t SpiComIF::performRegularSendOperation(SpiCookie* spiCookie, const
 #endif
       return result;
     }
+    updateLinePolarity(fileDescriptor);
     ReturnValue_t result = gpioComIF->pullLow(gpioId);
     if (result != HasReturnvaluesIF::RETURN_OK) {
 #if FSFW_VERBOSE_LEVEL >= 1
@@ -221,6 +220,8 @@ ReturnValue_t SpiComIF::performRegularSendOperation(SpiCookie* spiCookie, const
 #endif
       return result;
     }
+  } else {
+    updateLinePolarity(fileDescriptor);
   }
 
   /* Execute transfer */
@@ -250,7 +251,7 @@ ReturnValue_t SpiComIF::performRegularSendOperation(SpiCookie* spiCookie, const
 
   if (gpioId != gpio::NO_GPIO) {
     gpioComIF->pullHigh(gpioId);
-    result = spiMutex->unlockMutex();
+    result = csMutex->unlockMutex();
     if (result != RETURN_OK) {
 #if FSFW_CPP_OSTREAM_ENABLED == 1
       sif::error << "SpiComIF::sendMessage: Failed to unlock mutex" << std::endl;
@@ -278,9 +279,8 @@ ReturnValue_t SpiComIF::requestReceiveMessage(CookieIF* cookie, size_t requestLe
 
 ReturnValue_t SpiComIF::performHalfDuplexReception(SpiCookie* spiCookie) {
   ReturnValue_t result = HasReturnvaluesIF::RETURN_OK;
-  std::string device = spiCookie->getSpiDevice();
   int fileDescriptor = 0;
-  UnixFileGuard fileHelper(device, &fileDescriptor, O_RDWR, "SpiComIF::requestReceiveMessage");
+  UnixFileGuard fileHelper(dev, &fileDescriptor, O_RDWR, "SpiComIF::requestReceiveMessage");
   if (fileHelper.getOpenResult() != HasReturnvaluesIF::RETURN_OK) {
     return OPENING_FILE_FAILED;
   }
@@ -294,7 +294,7 @@ ReturnValue_t SpiComIF::performHalfDuplexReception(SpiCookie* spiCookie) {
 
   gpioId_t gpioId = spiCookie->getChipSelectPin();
   if (gpioId != gpio::NO_GPIO) {
-    result = spiMutex->lockMutex(timeoutType, timeoutMs);
+    result = csMutex->lockMutex(timeoutType, timeoutMs);
     if (result != RETURN_OK) {
 #if FSFW_CPP_OSTREAM_ENABLED == 1
       sif::error << "SpiComIF::getSendSuccess: Failed to lock mutex" << std::endl;
@@ -317,7 +317,7 @@ ReturnValue_t SpiComIF::performHalfDuplexReception(SpiCookie* spiCookie) {
 
   if (gpioId != gpio::NO_GPIO) {
     gpioComIF->pullHigh(gpioId);
-    result = spiMutex->unlockMutex();
+    result = csMutex->unlockMutex();
     if (result != RETURN_OK) {
 #if FSFW_CPP_OSTREAM_ENABLED == 1
       sif::error << "SpiComIF::getSendSuccess: Failed to unlock mutex" << std::endl;
@@ -353,7 +353,7 @@ MutexIF* SpiComIF::getMutex(MutexIF::TimeoutType* timeoutType, uint32_t* timeout
   if (timeoutMs != nullptr) {
     *timeoutMs = this->timeoutMs;
   }
-  return spiMutex;
+  return csMutex;
 }
 
 void SpiComIF::performSpiWiretapping(SpiCookie* spiCookie) {
@@ -401,14 +401,6 @@ void SpiComIF::setSpiSpeedAndMode(int spiFd, spi::SpiModes mode, uint32_t speed)
   if (retval != 0) {
     utility::handleIoctlError("SpiComIF::setSpiSpeedAndMode: Setting SPI speed failed");
   }
-  // This updates the SPI clock default polarity. Only setting the mode does not update
-  // the line state, which can be an issue on mode switches because the clock line will
-  // switch the state after the chip select is pulled low
-  clockUpdateTransfer.len = 0;
-  retval = ioctl(spiFd, SPI_IOC_MESSAGE(1), &clockUpdateTransfer);
-  if (retval != 0) {
-    utility::handleIoctlError("SpiComIF::setSpiSpeedAndMode: Updating SPI default clock failed");
-  }
 }
 
 void SpiComIF::getSpiSpeedAndMode(int spiFd, spi::SpiModes& mode, uint32_t& speed) const {
@@ -424,3 +416,15 @@ void SpiComIF::getSpiSpeedAndMode(int spiFd, spi::SpiModes &mode, uint32_t &spee
     utility::handleIoctlError("SpiComIF::getSpiSpeedAndMode: Getting SPI speed failed");
   }
 }
+
+const std::string& SpiComIF::getSpiDev() const {
+  return dev;
+}
+
+void SpiComIF::updateLinePolarity(int spiFd) {
+  clockUpdateTransfer.len = 0;
+  int retval = ioctl(spiFd, SPI_IOC_MESSAGE(1), &clockUpdateTransfer);
+  if (retval != 0) {
+    utility::handleIoctlError("SpiComIF::setSpiSpeedAndMode: Updating SPI default clock failed");
+  }
+}

hal/src/fsfw_hal/linux/spi/SpiComIF.h

@@ -32,7 +32,7 @@ class SpiComIF : public DeviceCommunicationIF, public SystemObject {
   static constexpr ReturnValue_t HALF_DUPLEX_TRANSFER_FAILED =
       HasReturnvaluesIF::makeReturnCode(spiRetvalId, 2);
 
-  SpiComIF(object_id_t objectId, GpioIF* gpioComIF);
+  SpiComIF(object_id_t objectId, std::string devname, GpioIF* gpioComIF);
 
   ReturnValue_t initializeInterface(CookieIF* cookie) override;
   ReturnValue_t sendMessage(CookieIF* cookie, const uint8_t* sendData, size_t sendLen) override;
@@ -59,7 +59,20 @@ class SpiComIF : public DeviceCommunicationIF, public SystemObject {
 
   GpioIF* getGpioInterface();
   void setSpiSpeedAndMode(int spiFd, spi::SpiModes mode, uint32_t speed);
+
+  /**
+   * This updates the SPI clock default polarity. Only setting the mode does not update
+   * the line state, which can be an issue on mode switches because the clock line will
+   * switch the state after the chip select is pulled low.
+   *
+   * It is recommended to call this function after #setSpiSpeedAndMode and after locking the
+   * CS mutex if the SPI bus has multiple SPI devices with different speed and SPI modes attached.
+   * @param spiFd
+   */
+  void updateLinePolarity(int spiFd);
   void getSpiSpeedAndMode(int spiFd, spi::SpiModes& mode, uint32_t& speed) const;
+
+  const std::string& getSpiDev() const;
   void performSpiWiretapping(SpiCookie* spiCookie);
 
   ReturnValue_t getReadBuffer(address_t spiAddress, uint8_t** buffer);
@@ -71,8 +84,12 @@ class SpiComIF : public DeviceCommunicationIF, public SystemObject {
   };
 
   GpioIF* gpioComIF = nullptr;
-
+  std::string dev = "";
-  MutexIF* spiMutex = nullptr;
+  /**
+   * Protects the chip select operations. Lock when GPIO is pulled low, unlock after it was
+   * pulled high
+   */
+  MutexIF* csMutex = nullptr;
   MutexIF::TimeoutType timeoutType = MutexIF::TimeoutType::WAITING;
   uint32_t timeoutMs = 20;
   spi_ioc_transfer clockUpdateTransfer = {};

hal/src/fsfw_hal/linux/spi/SpiCookie.cpp

@@ -1,26 +1,25 @@
 #include "SpiCookie.h"
 
-SpiCookie::SpiCookie(address_t spiAddress, gpioId_t chipSelect, std::string spiDev,
+SpiCookie::SpiCookie(address_t spiAddress, gpioId_t chipSelect, const size_t maxSize,
-                     const size_t maxSize, spi::SpiModes spiMode, uint32_t spiSpeed)
-    : SpiCookie(spi::SpiComIfModes::REGULAR, spiAddress, chipSelect, spiDev, maxSize, spiMode,
-                spiSpeed, nullptr, nullptr) {}
-
-SpiCookie::SpiCookie(address_t spiAddress, std::string spiDev, const size_t maxSize,
                      spi::SpiModes spiMode, uint32_t spiSpeed)
-    : SpiCookie(spiAddress, gpio::NO_GPIO, spiDev, maxSize, spiMode, spiSpeed) {}
+    : SpiCookie(spi::SpiComIfModes::REGULAR, spiAddress, chipSelect, maxSize, spiMode, spiSpeed,
+                nullptr, nullptr) {}
 
-SpiCookie::SpiCookie(address_t spiAddress, gpioId_t chipSelect, std::string spiDev,
+SpiCookie::SpiCookie(address_t spiAddress, const size_t maxSize, spi::SpiModes spiMode,
-                     const size_t maxSize, spi::SpiModes spiMode, uint32_t spiSpeed,
+                     uint32_t spiSpeed)
+    : SpiCookie(spiAddress, gpio::NO_GPIO, maxSize, spiMode, spiSpeed) {}
+
+SpiCookie::SpiCookie(address_t spiAddress, gpioId_t chipSelect, const size_t maxSize,
+                     spi::SpiModes spiMode, uint32_t spiSpeed,
                      spi::send_callback_function_t callback, void* args)
-    : SpiCookie(spi::SpiComIfModes::CALLBACK, spiAddress, chipSelect, spiDev, maxSize, spiMode,
+    : SpiCookie(spi::SpiComIfModes::CALLBACK, spiAddress, chipSelect, maxSize, spiMode, spiSpeed,
-                spiSpeed, callback, args) {}
+                callback, args) {}
 
 SpiCookie::SpiCookie(spi::SpiComIfModes comIfMode, address_t spiAddress, gpioId_t chipSelect,
-                     std::string spiDev, const size_t maxSize, spi::SpiModes spiMode,
+                     const size_t maxSize, spi::SpiModes spiMode, uint32_t spiSpeed,
-                     uint32_t spiSpeed, spi::send_callback_function_t callback, void* args)
+                     spi::send_callback_function_t callback, void* args)
     : spiAddress(spiAddress),
       chipSelectPin(chipSelect),
-      spiDevice(spiDev),
       comIfMode(comIfMode),
       maxSize(maxSize),
       spiMode(spiMode),
@@ -50,8 +49,6 @@ size_t SpiCookie::getMaxBufferSize() const { return maxSize; }
 
 address_t SpiCookie::getSpiAddress() const { return spiAddress; }
 
-std::string SpiCookie::getSpiDevice() const { return spiDevice; }
-
 void SpiCookie::setThreeWireSpi(bool enable) { uncommonParameters.threeWireSpi = enable; }
 
 void SpiCookie::setLsbFirst(bool enable) { uncommonParameters.lsbFirst = enable; }

hal/src/fsfw_hal/linux/spi/SpiCookie.h

@@ -29,23 +29,22 @@ class SpiCookie : public CookieIF {
    * @param spiDev
    * @param maxSize
    */
-  SpiCookie(address_t spiAddress, gpioId_t chipSelect, std::string spiDev, const size_t maxSize,
+  SpiCookie(address_t spiAddress, gpioId_t chipSelect, const size_t maxSize, spi::SpiModes spiMode,
-            spi::SpiModes spiMode, uint32_t spiSpeed);
+            uint32_t spiSpeed);
 
   /**
    * Like constructor above, but without a dedicated GPIO CS. Can be used for hardware
    * slave select or if CS logic is performed with decoders.
    */
-  SpiCookie(address_t spiAddress, std::string spiDev, const size_t maxReplySize,
+  SpiCookie(address_t spiAddress, const size_t maxReplySize, spi::SpiModes spiMode,
-            spi::SpiModes spiMode, uint32_t spiSpeed);
+            uint32_t spiSpeed);
 
   /**
    * Use the callback mode of the SPI communication interface. The user can pass the callback
    * function here or by using the setter function #setCallbackMode
    */
-  SpiCookie(address_t spiAddress, gpioId_t chipSelect, std::string spiDev, const size_t maxSize,
+  SpiCookie(address_t spiAddress, gpioId_t chipSelect, const size_t maxSize, spi::SpiModes spiMode,
-            spi::SpiModes spiMode, uint32_t spiSpeed, spi::send_callback_function_t callback,
+            uint32_t spiSpeed, spi::send_callback_function_t callback, void* args);
-            void* args);
 
   /**
    * Get the callback function
@@ -55,7 +54,6 @@ class SpiCookie : public CookieIF {
   void getCallback(spi::send_callback_function_t* callback, void** args);
 
   address_t getSpiAddress() const;
-  std::string getSpiDevice() const;
   gpioId_t getChipSelectPin() const;
   size_t getMaxBufferSize() const;
 
@@ -154,12 +152,11 @@ class SpiCookie : public CookieIF {
    * @param args
    */
   SpiCookie(spi::SpiComIfModes comIfMode, address_t spiAddress, gpioId_t chipSelect,
-            std::string spiDev, const size_t maxSize, spi::SpiModes spiMode, uint32_t spiSpeed,
+            const size_t maxSize, spi::SpiModes spiMode, uint32_t spiSpeed,
             spi::send_callback_function_t callback, void* args);
 
   address_t spiAddress;
   gpioId_t chipSelectPin;
-  std::string spiDevice;
 
   spi::SpiComIfModes comIfMode;
 

src/fsfw/container/FixedArrayList.h

@@ -12,6 +12,7 @@ template <typename T, size_t MAX_SIZE, typename count_t = uint8_t>
 class FixedArrayList : public ArrayList<T, count_t> {
   static_assert(MAX_SIZE <= std::numeric_limits<count_t>::max(),
                 "count_t is not large enough to hold MAX_SIZE");
+
  private:
   T data[MAX_SIZE];
 

src/fsfw/devicehandlers/DeviceHandlerBase.cpp

@@ -1401,8 +1401,7 @@ uint8_t DeviceHandlerBase::getReplyDelayCycles(DeviceCommandId_t deviceCommand)
   DeviceReplyMap::iterator iter = deviceReplyMap.find(deviceCommand);
   if (iter == deviceReplyMap.end()) {
     return 0;
-  }
+  } else if (iter->second.countdown != nullptr) {
-  else if (iter->second.countdown != nullptr) {
     return 0;
   }
   return iter->second.delayCycles;

src/fsfw/pus/CService201HealthCommanding.cpp

@@ -97,7 +97,8 @@ ReturnValue_t CService201HealthCommanding::handleReply(const CommandMessage *rep
 }
 
 // Not used for now, health state already reported by event
-[[maybe_unused]] ReturnValue_t CService201HealthCommanding::prepareHealthSetReply(const CommandMessage *reply) {
+[[maybe_unused]] ReturnValue_t CService201HealthCommanding::prepareHealthSetReply(
+    const CommandMessage *reply) {
   auto health = static_cast<uint8_t>(HealthMessage::getHealth(reply));
   auto oldHealth = static_cast<uint8_t>(HealthMessage::getOldHealth(reply));
   HealthSetReply healthSetReply(health, oldHealth);

src/fsfw/timemanager/Countdown.cpp

@@ -3,8 +3,7 @@
 Countdown::Countdown(uint32_t initialTimeout, bool startImmediately) : timeout(initialTimeout) {
   if (startImmediately) {
     setTimeout(initialTimeout);
-  }
+  } else {
-  else {
     timeout = initialTimeout;
   }
 }

src/fsfw/tmtcservices/SourceSequenceCounter.h

@@ -5,10 +5,10 @@
 
 class SourceSequenceCounter {
  private:
-  uint16_t sequenceCount;
+  uint16_t sequenceCount = 0;
 
  public:
-  SourceSequenceCounter() : sequenceCount(0) {}
+  SourceSequenceCounter(uint16_t initialSequenceCount = 0) : sequenceCount(initialSequenceCount) {}
   void increment() {
     sequenceCount = (sequenceCount + 1) % (SpacePacketBase::LIMIT_SEQUENCE_COUNT);
   }
@@ -31,6 +31,7 @@ class SourceSequenceCounter {
     sequenceCount = newCount;
     return *this;
   }
+
   operator uint16_t() { return this->get(); }
 };