From b5b80cef972f054fd0995c4370904a06261d1188 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 21 Feb 2023 02:59:28 +0100 Subject: [PATCH] fixed regression in max31865 code --- CHANGELOG.md | 4 + fsfw | 2 +- linux/ObjectFactory.cpp | 4 +- linux/devices/CMakeLists.txt | 2 +- ...evelHandler.cpp => Max31865RtdPolling.cpp} | 84 +++++++++---------- ...LowlevelHandler.h => Max31865RtdPolling.h} | 8 +- 6 files changed, 52 insertions(+), 52 deletions(-) rename linux/devices/{Max31865RtdLowlevelHandler.cpp => Max31865RtdPolling.cpp} (81%) rename linux/devices/{Max31865RtdLowlevelHandler.h => Max31865RtdPolling.h} (92%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37503174..4a397de6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ change warranting a new major release: - CFDP Funnel bugfix: CCSDS wrapping was buggy and works properly now. - CMakeLists.txt fix which broke CI/CD builds when server could not retrieve full git SHA. +- Possible regression in the MAX31865 polling task: Using a `ManualCsLockGuard` for reconfiguring + and then polling the sensor is problematic, invalid sensor values will be read. + CS probably needs to be de-asserted or some other HW/SPI specific issue. Letting the SPI ComIF + do the locking does the job. ## Changed diff --git a/fsfw b/fsfw index c3279852..2efff4d2 160000 --- a/fsfw +++ b/fsfw @@ -1 +1 @@ -Subproject commit c32798522283d89028b7c1ecd3bd33b8391e1a39 +Subproject commit 2efff4d2c5ef82b5b62567ab1bb0ee53aeed6a5a diff --git a/linux/ObjectFactory.cpp b/linux/ObjectFactory.cpp index 1686eb03..e3162a07 100644 --- a/linux/ObjectFactory.cpp +++ b/linux/ObjectFactory.cpp @@ -9,7 +9,7 @@ #include #include #include -#include +#include #include #include #include @@ -278,7 +278,7 @@ void ObjectFactory::createRtdComponents(std::string spiDev, GpioIF* gpioComIF, TcsBoardAssembly* tcsBoardAss = ObjectFactory::createTcsBoardAssy(*pwrSwitcher); // Create special low level reader communication interface - new Max31865RtdReader(objects::SPI_RTD_COM_IF, comIF, gpioComIF); + new Max31865RtdPolling(objects::SPI_RTD_COM_IF, comIF, gpioComIF); for (uint8_t idx = 0; idx < NUM_RTDS; idx++) { rtdCookies[idx] = new SpiCookie(cookieArgs[idx].first, cookieArgs[idx].second, MAX31865::MAX_REPLY_SIZE, spi::RTD_MODE, spi::RTD_SPEED); diff --git a/linux/devices/CMakeLists.txt b/linux/devices/CMakeLists.txt index 7251b802..58b6d021 100644 --- a/linux/devices/CMakeLists.txt +++ b/linux/devices/CMakeLists.txt @@ -3,7 +3,7 @@ if(EIVE_BUILD_GPSD_GPS_HANDLER) endif() target_sources( - ${OBSW_NAME} PRIVATE Max31865RtdLowlevelHandler.cpp ScexUartReader.cpp + ${OBSW_NAME} PRIVATE Max31865RtdPolling.cpp ScexUartReader.cpp ScexDleParser.cpp ScexHelper.cpp RwPollingTask.cpp) add_subdirectory(ploc) diff --git a/linux/devices/Max31865RtdLowlevelHandler.cpp b/linux/devices/Max31865RtdPolling.cpp similarity index 81% rename from linux/devices/Max31865RtdLowlevelHandler.cpp rename to linux/devices/Max31865RtdPolling.cpp index 0af22bfa..a22ec145 100644 --- a/linux/devices/Max31865RtdLowlevelHandler.cpp +++ b/linux/devices/Max31865RtdPolling.cpp @@ -1,8 +1,7 @@ -#include "Max31865RtdLowlevelHandler.h" - #include #include #include +#include #define OBSW_RTD_AUTO_MODE 1 @@ -17,16 +16,17 @@ static constexpr uint8_t BASE_CFG = (MAX31865::ConvMode::NORM_OFF << MAX31865::CfgBitPos::CONV_MODE); #endif -Max31865RtdReader::Max31865RtdReader(object_id_t objectId, SpiComIF* lowLevelComIF, GpioIF* gpioIF) +Max31865RtdPolling::Max31865RtdPolling(object_id_t objectId, SpiComIF* lowLevelComIF, + GpioIF* gpioIF) : SystemObject(objectId), rtds(EiveMax31855::NUM_RTDS), comIF(lowLevelComIF), gpioIF(gpioIF) { readerMutex = MutexFactory::instance()->createMutex(); } -ReturnValue_t Max31865RtdReader::performOperation(uint8_t operationCode) { +ReturnValue_t Max31865RtdPolling::performOperation(uint8_t operationCode) { using namespace MAX31865; ReturnValue_t result = returnvalue::OK; static_cast(result); - // Stopwatch watch; + Stopwatch watch; if (periodicInitHandling()) { #if OBSW_RTD_AUTO_MODE == 0 // 10 ms delay for VBIAS startup @@ -49,17 +49,16 @@ ReturnValue_t Max31865RtdReader::performOperation(uint8_t operationCode) { return periodicReadHandling(); } -bool Max31865RtdReader::rtdIsActive(uint8_t idx) { +bool Max31865RtdPolling::rtdIsActive(uint8_t idx) { if (rtds[idx]->on and rtds[idx]->db.active and rtds[idx]->db.configured) { return true; } return false; } -bool Max31865RtdReader::periodicInitHandling() { +bool Max31865RtdPolling::periodicInitHandling() { using namespace MAX31865; ReturnValue_t result = returnvalue::OK; - for (auto& rtd : rtds) { if (rtd == nullptr) { continue; @@ -70,11 +69,9 @@ bool Max31865RtdReader::periodicInitHandling() { return false; } if ((rtd->on or rtd->db.active) and not rtd->db.configured and rtd->cd.hasTimedOut()) { - ManualCsLockWrapper mg1(csLock, gpioIF, rtd->spiCookie, csTimeoutType, csTimeoutMs); - if (mg1.lockResult != returnvalue::OK or mg1.gpioResult != returnvalue::OK) { - sif::error << "Max31865RtdReader::periodicInitHandling: Manual CS lock failed" << std::endl; - continue; - } + // Please note that using the manual CS lock wrapper here is problematic. Might be a SPI + // or hardware specific issue where the CS needs to be pulled high and then low again + // between transfers result = writeCfgReg(rtd->spiCookie, BASE_CFG); if (result != returnvalue::OK) { handleSpiError(rtd, result, "writeCfgReg"); @@ -115,7 +112,7 @@ bool Max31865RtdReader::periodicInitHandling() { return someRtdUsable; } -ReturnValue_t Max31865RtdReader::periodicReadReqHandling() { +ReturnValue_t Max31865RtdPolling::periodicReadReqHandling() { using namespace MAX31865; // Now request one shot config for all active RTDs for (auto& rtd : rtds) { @@ -139,7 +136,7 @@ ReturnValue_t Max31865RtdReader::periodicReadReqHandling() { return returnvalue::OK; } -ReturnValue_t Max31865RtdReader::periodicReadHandling() { +ReturnValue_t Max31865RtdPolling::periodicReadHandling() { using namespace MAX31865; auto result = returnvalue::OK; // Now read the RTD values @@ -153,11 +150,9 @@ ReturnValue_t Max31865RtdReader::periodicReadHandling() { return returnvalue::FAILED; } if (rtdIsActive(rtd->idx)) { - ManualCsLockWrapper mg1(csLock, gpioIF, rtd->spiCookie, csTimeoutType, csTimeoutMs); - if (mg1.lockResult != returnvalue::OK or mg1.gpioResult != returnvalue::OK) { - sif::error << "Max31865RtdReader::periodicInitHandling: Manual CS lock failed" << std::endl; - continue; - } + // Please note that using the manual CS lock wrapper here is problematic. Might be a SPI + // or hardware specific issue where the CS needs to be pulled high and then low again + // between transfers uint16_t rtdVal = 0; bool faultBitSet = false; result = writeCfgReg(rtd->spiCookie, BASE_CFG); @@ -166,6 +161,7 @@ ReturnValue_t Max31865RtdReader::periodicReadHandling() { continue; } result = readRtdVal(rtd->spiCookie, rtdVal, faultBitSet); + // sif::debug << "RTD Val: " << rtdVal << std::endl; if (result != returnvalue::OK) { handleSpiError(rtd, result, "readRtdVal"); continue; @@ -191,7 +187,7 @@ ReturnValue_t Max31865RtdReader::periodicReadHandling() { return returnvalue::OK; } -ReturnValue_t Max31865RtdReader::initializeInterface(CookieIF* cookie) { +ReturnValue_t Max31865RtdPolling::initializeInterface(CookieIF* cookie) { if (cookie == nullptr) { throw std::invalid_argument("Invalid MAX31865 Reader Cookie"); } @@ -211,8 +207,8 @@ ReturnValue_t Max31865RtdReader::initializeInterface(CookieIF* cookie) { return returnvalue::OK; } -ReturnValue_t Max31865RtdReader::sendMessage(CookieIF* cookie, const uint8_t* sendData, - size_t sendLen) { +ReturnValue_t Max31865RtdPolling::sendMessage(CookieIF* cookie, const uint8_t* sendData, + size_t sendLen) { if (cookie == nullptr) { return returnvalue::FAILED; } @@ -308,14 +304,14 @@ ReturnValue_t Max31865RtdReader::sendMessage(CookieIF* cookie, const uint8_t* se return returnvalue::OK; } -ReturnValue_t Max31865RtdReader::getSendSuccess(CookieIF* cookie) { return returnvalue::OK; } +ReturnValue_t Max31865RtdPolling::getSendSuccess(CookieIF* cookie) { return returnvalue::OK; } -ReturnValue_t Max31865RtdReader::requestReceiveMessage(CookieIF* cookie, size_t requestLen) { +ReturnValue_t Max31865RtdPolling::requestReceiveMessage(CookieIF* cookie, size_t requestLen) { return returnvalue::OK; } -ReturnValue_t Max31865RtdReader::readReceivedMessage(CookieIF* cookie, uint8_t** buffer, - size_t* size) { +ReturnValue_t Max31865RtdPolling::readReceivedMessage(CookieIF* cookie, uint8_t** buffer, + size_t* size) { MutexGuard mg(readerMutex); if (mg.getLockResult() != returnvalue::OK) { // TODO: Emit warning @@ -338,13 +334,13 @@ ReturnValue_t Max31865RtdReader::readReceivedMessage(CookieIF* cookie, uint8_t** return returnvalue::OK; } -ReturnValue_t Max31865RtdReader::writeCfgReg(SpiCookie* cookie, uint8_t cfg) { +ReturnValue_t Max31865RtdPolling::writeCfgReg(SpiCookie* cookie, uint8_t cfg) { using namespace MAX31865; return writeNToReg(cookie, CONFIG, 1, &cfg, nullptr); } -ReturnValue_t Max31865RtdReader::writeBiasSel(MAX31865::Bias bias, SpiCookie* cookie, - uint8_t baseCfg) { +ReturnValue_t Max31865RtdPolling::writeBiasSel(MAX31865::Bias bias, SpiCookie* cookie, + uint8_t baseCfg) { using namespace MAX31865; if (bias == MAX31865::Bias::OFF) { baseCfg &= ~(1 << CfgBitPos::BIAS_SEL); @@ -354,7 +350,7 @@ ReturnValue_t Max31865RtdReader::writeBiasSel(MAX31865::Bias bias, SpiCookie* co return writeCfgReg(cookie, baseCfg); } -ReturnValue_t Max31865RtdReader::clearFaultStatus(SpiCookie* cookie) { +ReturnValue_t Max31865RtdPolling::clearFaultStatus(SpiCookie* cookie) { using namespace MAX31865; // Read back the current configuration to avoid overwriting it when clearing te fault status uint8_t currentCfg = 0; @@ -368,7 +364,7 @@ ReturnValue_t Max31865RtdReader::clearFaultStatus(SpiCookie* cookie) { return writeCfgReg(cookie, currentCfg); } -ReturnValue_t Max31865RtdReader::readCfgReg(SpiCookie* cookie, uint8_t& cfg) { +ReturnValue_t Max31865RtdPolling::readCfgReg(SpiCookie* cookie, uint8_t& cfg) { using namespace MAX31865; uint8_t* replyPtr = nullptr; auto result = readNFromReg(cookie, CONFIG, 1, &replyPtr); @@ -378,19 +374,19 @@ ReturnValue_t Max31865RtdReader::readCfgReg(SpiCookie* cookie, uint8_t& cfg) { return result; } -ReturnValue_t Max31865RtdReader::writeLowThreshold(SpiCookie* cookie, uint16_t val) { +ReturnValue_t Max31865RtdPolling::writeLowThreshold(SpiCookie* cookie, uint16_t val) { using namespace MAX31865; uint8_t cmd[2] = {static_cast((val >> 8) & 0xff), static_cast(val & 0xff)}; return writeNToReg(cookie, LOW_THRESHOLD, 2, cmd, nullptr); } -ReturnValue_t Max31865RtdReader::writeHighThreshold(SpiCookie* cookie, uint16_t val) { +ReturnValue_t Max31865RtdPolling::writeHighThreshold(SpiCookie* cookie, uint16_t val) { using namespace MAX31865; uint8_t cmd[2] = {static_cast((val >> 8) & 0xff), static_cast(val & 0xff)}; return writeNToReg(cookie, HIGH_THRESHOLD, 2, cmd, nullptr); } -ReturnValue_t Max31865RtdReader::readLowThreshold(SpiCookie* cookie, uint16_t& lowThreshold) { +ReturnValue_t Max31865RtdPolling::readLowThreshold(SpiCookie* cookie, uint16_t& lowThreshold) { using namespace MAX31865; uint8_t* replyPtr = nullptr; auto result = readNFromReg(cookie, LOW_THRESHOLD, 2, &replyPtr); @@ -400,7 +396,7 @@ ReturnValue_t Max31865RtdReader::readLowThreshold(SpiCookie* cookie, uint16_t& l return result; } -ReturnValue_t Max31865RtdReader::readHighThreshold(SpiCookie* cookie, uint16_t& highThreshold) { +ReturnValue_t Max31865RtdPolling::readHighThreshold(SpiCookie* cookie, uint16_t& highThreshold) { using namespace MAX31865; uint8_t* replyPtr = nullptr; auto result = readNFromReg(cookie, HIGH_THRESHOLD, 2, &replyPtr); @@ -410,8 +406,8 @@ ReturnValue_t Max31865RtdReader::readHighThreshold(SpiCookie* cookie, uint16_t& return result; } -ReturnValue_t Max31865RtdReader::writeNToReg(SpiCookie* cookie, uint8_t reg, size_t n, uint8_t* cmd, - uint8_t** reply) { +ReturnValue_t Max31865RtdPolling::writeNToReg(SpiCookie* cookie, uint8_t reg, size_t n, + uint8_t* cmd, uint8_t** reply) { using namespace MAX31865; if (n > cmdBuf.size() - 1) { return returnvalue::FAILED; @@ -423,7 +419,7 @@ ReturnValue_t Max31865RtdReader::writeNToReg(SpiCookie* cookie, uint8_t reg, siz return comIF->sendMessage(cookie, cmdBuf.data(), n + 1); } -ReturnValue_t Max31865RtdReader::readRtdVal(SpiCookie* cookie, uint16_t& val, bool& faultBitSet) { +ReturnValue_t Max31865RtdPolling::readRtdVal(SpiCookie* cookie, uint16_t& val, bool& faultBitSet) { using namespace MAX31865; uint8_t* replyPtr = nullptr; auto result = readNFromReg(cookie, RTD, 2, &replyPtr); @@ -438,8 +434,8 @@ ReturnValue_t Max31865RtdReader::readRtdVal(SpiCookie* cookie, uint16_t& val, bo return result; } -ReturnValue_t Max31865RtdReader::readNFromReg(SpiCookie* cookie, uint8_t reg, size_t n, - uint8_t** reply) { +ReturnValue_t Max31865RtdPolling::readNFromReg(SpiCookie* cookie, uint8_t reg, size_t n, + uint8_t** reply) { using namespace MAX31865; if (n > 4) { return returnvalue::FAILED; @@ -465,15 +461,15 @@ ReturnValue_t Max31865RtdReader::readNFromReg(SpiCookie* cookie, uint8_t reg, si return returnvalue::OK; } -ReturnValue_t Max31865RtdReader::handleSpiError(Max31865ReaderCookie* cookie, ReturnValue_t result, - const char* ctx) { +ReturnValue_t Max31865RtdPolling::handleSpiError(Max31865ReaderCookie* cookie, ReturnValue_t result, + const char* ctx) { cookie->db.spiErrorCount.value += 1; sif::warning << "Max31865RtdReader::handleSpiError: " << ctx << " | Failed with result " << result << std::endl; return result; } -ReturnValue_t Max31865RtdReader::initialize() { +ReturnValue_t Max31865RtdPolling::initialize() { csLock = comIF->getCsMutex(); return SystemObject::initialize(); } diff --git a/linux/devices/Max31865RtdLowlevelHandler.h b/linux/devices/Max31865RtdPolling.h similarity index 92% rename from linux/devices/Max31865RtdLowlevelHandler.h rename to linux/devices/Max31865RtdPolling.h index 4854ef3b..a34c8e53 100644 --- a/linux/devices/Max31865RtdLowlevelHandler.h +++ b/linux/devices/Max31865RtdPolling.h @@ -35,11 +35,11 @@ struct Max31865ReaderCookie : public CookieIF { EiveMax31855::ReadOutStruct db; }; -class Max31865RtdReader : public SystemObject, - public ExecutableObjectIF, - public DeviceCommunicationIF { +class Max31865RtdPolling : public SystemObject, + public ExecutableObjectIF, + public DeviceCommunicationIF { public: - Max31865RtdReader(object_id_t objectId, SpiComIF* lowLevelComIF, GpioIF* gpioIF); + Max31865RtdPolling(object_id_t objectId, SpiComIF* lowLevelComIF, GpioIF* gpioIF); ReturnValue_t performOperation(uint8_t operationCode) override; ReturnValue_t initialize() override;