From 279c7c45110f1e3320ad29f4916fb79ea1f9e84f Mon Sep 17 00:00:00 2001 From: Rob Dobson Date: Sun, 25 Oct 2026 16:03:17 +0100 Subject: [PATCH 1/2] File uploads: check the file against the CRC16 given in ufStart ufStart has an optional CRC16, which was parsed and passed along with every block but never checked, so clients checked uploads with a separate filehash request. FileStreamSession now keeps a CRC16-CCITT (as filehash reports it) of the data as it is written, in order - on the file system worker for background writes - and after the final block compares it with the CRC16 given. A mismatch fails the upload with the new RAFT_CRC_MISMATCH: the file is deleted and the client gets a ufCancel with reason failCRC instead of the final ack (or a ufStatus CRCMismatch reply when blocks are written as they arrive). When the CRC was given and matched, the ufEnd reply includes it ("crc16":"xxxx"), so a client knows the device checked the file and can skip filehash; older firmware doesn't, so clients that see no crc16 in the reply still check. Firmware updates aren't checked here (ESP-IDF validates the image). Co-Authored-By: Claude Opus 5.5 --- .../FileUploadOKTOProtocol.cpp | 24 ++++- .../ProtocolExchange/FileStreamSession.cpp | 36 +++++++- .../ProtocolExchange/FileStreamSession.h | 8 ++ components/core/Utils/RaftRetCode.h | 1 + components/core/Utils/RaftUtils.cpp | 1 + .../test_file_upload_background.cpp | 87 ++++++++++++++++++- 6 files changed, 149 insertions(+), 8 deletions(-) diff --git a/components/comms/FileStreamProtocols/FileUploadOKTOProtocol.cpp b/components/comms/FileStreamProtocols/FileUploadOKTOProtocol.cpp index 8671d155..572809e0 100644 --- a/components/comms/FileStreamProtocols/FileUploadOKTOProtocol.cpp +++ b/components/comms/FileStreamProtocols/FileUploadOKTOProtocol.cpp @@ -143,6 +143,11 @@ RaftRetCode FileUploadOKTOProtocol::handleDataFrame(const RICRESTMsg& ricRESTReq else transferCancel("failOTAWrite"); } + else if (rslt == RAFT_CRC_MISMATCH) + { + Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, false, R"("cmdName":"ufStatus","reason":"CRCMismatch")"); + transferCancel("failCRC"); + } else { Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, false, R"("cmdName":"ufStatus","reason":"FileWriteFailed")"); @@ -213,7 +218,10 @@ void FileUploadOKTOProtocol::loop() else if (writeStatus != RAFT_OK) { LOG_W(MODULE_PREFIX, "loop background write failed %s", Raft::getRetCodeStr(writeStatus)); - transferCancel(_fileStreamContentType == FILE_STREAM_CONTENT_TYPE_FIRMWARE ? "failOTAWrite" : "failFileWrite"); + if (_fileStreamContentType == FILE_STREAM_CONTENT_TYPE_FIRMWARE) + transferCancel("failOTAWrite"); + else + transferCancel(writeStatus == RAFT_CRC_MISMATCH ? "failCRC" : "failFileWrite"); return; } else if (_ackDeferred) @@ -404,8 +412,18 @@ RaftRetCode FileUploadOKTOProtocol::handleEndMsg(const RICRESTMsg& ricRESTReqMs if (_fileStreamCancelEnd) _fileStreamCancelEnd(true); - // Response - Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true); + // Response - with the CRC the file was checked against, if the client gave one (a file that didn't match + // ended the upload with a cancel, failCRC, before this) + if (_isUploading && _expCRC16Valid && (_fileStreamContentType != FILE_STREAM_CONTENT_TYPE_FIRMWARE)) + { + char crcJson[30]; + snprintf(crcJson, sizeof(crcJson), R"("crc16":"%04x")", (unsigned)(_expCRC16 & 0xffff)); + Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true, crcJson); + } + else + { + Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true); + } // Debug #ifdef DEBUG_RICREST_FILEUPLOAD diff --git a/components/comms/ProtocolExchange/FileStreamSession.cpp b/components/comms/ProtocolExchange/FileStreamSession.cpp index e3fc905d..d3e7ba83 100644 --- a/components/comms/ProtocolExchange/FileStreamSession.cpp +++ b/components/comms/ProtocolExchange/FileStreamSession.cpp @@ -15,6 +15,7 @@ #include "RestAPIEndpointManager.h" #include "RICRESTMsg.h" #include "MiniHDLC.h" +#include "FileSystem.h" #include "SpiramAwareAllocator.h" #include "FileSystemWorker.h" #include @@ -596,6 +597,14 @@ RaftRetCode FileStreamSession::writeFileBlock(FileStreamBlock& fileStreamBlock) if (!_pFileChunker) return RAFT_INVALID_OPERATION; + // The CRC to check the file against (given with every block; taken from the first) + if (fileStreamBlock.firstBlock) + { + _crcCheck = fileStreamBlock.crc16Valid; + _crcExpected = fileStreamBlock.crc16; + _crcWritten = MiniHDLC::crcInitCCITT(); + } + // Written here (not in the background) if (!_writeInBackground) { @@ -605,7 +614,9 @@ RaftRetCode FileStreamSession::writeFileBlock(FileStreamBlock& fileStreamBlock) bytesWritten, fileStreamBlock.finalBlock); _totalBytes += bytesWritten; _totalWriteTimeUs += micros() - startUs; - return chunkerRslt ? RAFT_OK : RAFT_OTHER_FAILURE; + if (!chunkerRslt) + return RAFT_OTHER_FAILURE; + return checkWrittenCRC(fileStreamBlock.pBlock, fileStreamBlock.blockLen, fileStreamBlock.finalBlock); } // Add the block to the data waiting to be written @@ -680,8 +691,10 @@ void FileStreamSession::writeJob() _writePendingFinal = false; RaftMutex_unlock(_writeMutex); - // Write it + // Write it (and check it against the expected CRC) RaftRetCode rslt = writeToFile(toWrite.data(), toWrite.size(), isFinal); + if (rslt == RAFT_OK) + rslt = checkWrittenCRC(toWrite.data(), toWrite.size(), isFinal); toWrite.clear(); if (rslt != RAFT_OK) { @@ -695,6 +708,25 @@ void FileStreamSession::writeJob() } } +///////////////////////////////////////////////////////////////////////////////////////////////////////////////// +// Update the CRC of the data written and, after the final block, compare it with the CRC the client gave +// (a file that doesn't match is deleted) +///////////////////////////////////////////////////////////////////////////////////////////////////////////////// + +RaftRetCode FileStreamSession::checkWrittenCRC(const uint8_t* pBuf, uint32_t len, bool isFinal) +{ + if (!_crcCheck) + return RAFT_OK; + if (pBuf && (len > 0)) + _crcWritten = MiniHDLC::crcUpdateCCITT(_crcWritten, pBuf, len); + if (!isFinal || (_crcWritten == _crcExpected)) + return RAFT_OK; + LOG_W(MODULE_PREFIX, "upload CRC mismatch filename %s expected %04x written %04x - file deleted", + _fileStreamName.c_str(), _crcExpected, _crcWritten); + fileSystem.deleteFile("", _fileStreamName); + return RAFT_CRC_MISMATCH; +} + ///////////////////////////////////////////////////////////////////////////////////////////////////////////////// // Write to the file in pieces (the file system lock is released between them), closing it after the final block ///////////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/components/comms/ProtocolExchange/FileStreamSession.h b/components/comms/ProtocolExchange/FileStreamSession.h index 69d6a53a..74ea8b8d 100644 --- a/components/comms/ProtocolExchange/FileStreamSession.h +++ b/components/comms/ProtocolExchange/FileStreamSession.h @@ -163,6 +163,14 @@ class FileStreamSession void writeJob(); RaftRetCode writeToFile(const uint8_t* pBuf, uint32_t len, bool isFinal); + // Check of an uploaded file against the CRC16 (CRC16-CCITT, as filehash reports) the client gave in ufStart: + // updated as the data is written, in order (on the file system worker for background writes), and compared + // after the final block. A mismatch fails the upload (RAFT_CRC_MISMATCH) and the file is deleted + bool _crcCheck = false; + uint16_t _crcExpected = 0; + uint16_t _crcWritten = 0; + RaftRetCode checkWrittenCRC(const uint8_t* pBuf, uint32_t len, bool isFinal); + // Background reads (file downloads): a block is read on the file system worker and the next one read ahead. // fileStreamBlockRead() returns RAFT_BUSY until the block asked for has been read. The job sets the _read... // results, then clears _readJobOutstanding; the main task only uses them when no job is outstanding diff --git a/components/core/Utils/RaftRetCode.h b/components/core/Utils/RaftRetCode.h index 4028fd59..3ec1acdf 100644 --- a/components/core/Utils/RaftRetCode.h +++ b/components/core/Utils/RaftRetCode.h @@ -51,4 +51,5 @@ enum RaftRetCode RAFT_MOTION_BUSY, RAFT_MOTION_HOMING_REQUIRED, RAFT_RESP_DEFERRED, + RAFT_CRC_MISMATCH, }; diff --git a/components/core/Utils/RaftUtils.cpp b/components/core/Utils/RaftUtils.cpp index 6b97567f..04d754d8 100644 --- a/components/core/Utils/RaftUtils.cpp +++ b/components/core/Utils/RaftUtils.cpp @@ -1528,6 +1528,7 @@ const char* Raft::getRetCodeStr(RaftRetCode retc) case RAFT_MOTION_NO_STEPS: return "MOTION_NO_STEPS"; case RAFT_MOTION_BUSY: return "MOTION_BUSY"; case RAFT_MOTION_HOMING_REQUIRED: return "MOTION_HOMING_REQUIRED"; + case RAFT_CRC_MISMATCH: return "CRC_MISMATCH"; default: return "UNKNOWN"; } }; diff --git a/linux_unit_tests/test_file_upload_background.cpp b/linux_unit_tests/test_file_upload_background.cpp index e5aea3b2..38cf1d25 100644 --- a/linux_unit_tests/test_file_upload_background.cpp +++ b/linux_unit_tests/test_file_upload_background.cpp @@ -5,7 +5,9 @@ // A FileStreamSession for a file upload hands the blocks to the file system worker (FileSystemWorker) and the // upload protocol acknowledges blocks only once they are written. The worker runs its jobs when the test calls // runPending(), so each step can be checked: blocks waiting, written, the acks held back and sent, the file closed -// after the final block, the session ending only then, and a cancelled upload dropping what wasn't written. A file +// after the final block, the session ending only then, and a cancelled upload dropping what wasn't written. An +// upload given a CRC16 in ufStart is checked against it once written: a mismatch cancels it (failCRC) and deletes +// the file, and a match is confirmed in the ufEnd reply. A file // download's blocks are read on the worker too (RAFT_BUSY until read), each one reading the next ahead. // ///////////////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -22,6 +24,7 @@ #include "FileStreamSession.h" #include "FileSystemWorker.h" #include "FileSystem.h" +#include "MiniHDLC.h" static const uint32_t TEST_CHANNEL_ID = 3; static const uint32_t BLOCK = 100; @@ -72,7 +75,8 @@ class Upload FileStreamSession* pSession = nullptr; String startResp; - Upload(uint32_t fileLen, const char* name) + // crc16Field: the CRC16 given in ufStart (e.g. "0x1234"), or nullptr for none + Upload(uint32_t fileLen, const char* name, const char* crc16Field = nullptr) { char cwd[512]; path = String(getcwd(cwd, sizeof(cwd))) + "/" + name; @@ -84,7 +88,8 @@ class Upload 1, "", nullptr, fileLen); String startJson = R"({"cmdName":"ufStart","reqStr":"fileupload","fileType":"fs","fileName":")" + path + R"(","fileLen":)" + String(fileLen) + R"(,"batchMsgSize":)" + String(BLOCK) + - R"(,"batchAckSize":)" + String(BATCH) + "}"; + R"(,"batchAckSize":)" + String(BATCH) + + (crc16Field ? String(R"(,"CRC16":")") + crc16Field + "\"" : String("")) + "}"; CommsChannelMsg msg(TEST_CHANNEL_ID, MSG_PROTOCOL_RICREST, 1, MSG_TYPE_COMMAND); RICRESTMsg::encode(startJson, msg, RICRESTMsg::RICREST_ELEM_CODE_COMMAND_FRAME); RICRESTMsg req; @@ -98,6 +103,47 @@ class Upload unlink(path.c_str()); } + // CRC16-CCITT of the contents, as filehash reports it, as "0x..." + String contentsCRC() const + { + char crcStr[10]; + snprintf(crcStr, sizeof(crcStr), "0x%04x", + MiniHDLC::crcUpdateCCITT(MiniHDLC::crcInitCCITT(), contents.data(), contents.size())); + return crcStr; + } + + // Send ufEnd; returns the reply + String sendEnd() + { + String endJson = R"({"cmdName":"ufEnd","reqStr":"fileupload","fileType":"fs","fileName":")" + path + + R"(","fileLen":)" + String(contents.size()) + "}"; + CommsChannelMsg msg(TEST_CHANNEL_ID, MSG_PROTOCOL_RICREST, 2, MSG_TYPE_COMMAND); + RICRESTMsg::encode(endJson, msg, RICRESTMsg::RICREST_ELEM_CODE_COMMAND_FRAME); + RICRESTMsg req; + req.decode(msg.getBuf(), msg.getBufLen()); + String resp; + pSession->handleCmdFrame(FileStreamBase::FILE_STREAM_MSG_TYPE_UPLOAD_END, req, resp, msg); + return resp; + } + + // Upload every block, running the worker after each batch; returns the reasons of any cancels sent + std::vector sendAll() + { + std::vector cancels; + uint32_t numBlocks = (contents.size() + BLOCK - 1) / BLOCK; + for (uint32_t i = 0; i < numBlocks; i++) + { + sendBlock(i); + fileSystemWorker.runPending(); + pSession->loop(); + for (const String& msg : commsCore.sent) + if (RaftJson(msg).getString("cmdName", "") == "ufCancel") + cancels.push_back(RaftJson(msg).getString("reason", "")); + commsCore.sent.clear(); + } + return cancels; + } + // Send the block at `blockIdx`; returns the okto in the reply, or -1 if there is none int32_t sendBlock(uint32_t blockIdx) { @@ -207,6 +253,40 @@ static void testCancelDropsUnwritten() TEST_ASSERT(up.fileContents().size() == BLOCK, "blocks waiting at the cancel not written"); } +static void testCRCChecked() +{ + printf("Running testCRCChecked...\n"); + + // The right CRC: the upload completes and ufEnd confirms the CRC checked + { + Upload probe(12 * BLOCK - 30, "bg_upload_crc.bin"); + String crc = probe.contentsCRC(); + delete probe.pSession; + probe.pSession = nullptr; + Upload up(12 * BLOCK - 30, "bg_upload_crc.bin", crc.c_str()); + TEST_ASSERT(up.sendAll().empty(), "no cancel with the right CRC"); + TEST_ASSERT(up.fileContents() == up.contents, "file written"); + String endResp = up.sendEnd(); + TEST_ASSERT(RaftJson(endResp).getString("rslt", "") == "ok", "ufEnd ok"); + TEST_ASSERT(String("0x") + RaftJson(endResp).getString("crc16", "") == crc, "ufEnd gives the CRC checked"); + } + + // A wrong CRC: cancelled once the final block is written (failCRC), and the file deleted + { + Upload up(12 * BLOCK - 30, "bg_upload_crc.bin", "0x1234"); + std::vector cancels = up.sendAll(); + TEST_ASSERT(cancels == std::vector({"failCRC"}), "cancelled with failCRC"); + TEST_ASSERT(up.fileContents().empty(), "file deleted"); + } + + // No CRC given: no check, and none in the ufEnd reply + { + Upload up(3 * BLOCK, "bg_upload_crc.bin"); + TEST_ASSERT(up.sendAll().empty(), "no cancel without a CRC"); + TEST_ASSERT(RaftJson(up.sendEnd()).getString("crc16", "none") == "none", "no CRC in ufEnd"); + } +} + static void testWorkerRunsJobsInOrder() { printf("Running testWorkerRunsJobsInOrder...\n"); @@ -282,6 +362,7 @@ int main() testWorkerRunsJobsInOrder(); testBlocksWrittenByWorkerThenAcked(); testCancelDropsUnwritten(); + testCRCChecked(); testDownloadBlocksReadByWorker(); return testSummary("test_file_upload_background"); } From 97f41587e1378685b0b77f5ac49aa9ace0174475 Mon Sep 17 00:00:00 2001 From: Rob Dobson Date: Sun, 25 Oct 2026 16:06:33 +0100 Subject: [PATCH 2/2] File uploads: the ufEnd reply says when the file was checked against its CRC The upload session ends once the final block is written, so ufEnd usually finds no session (and got a plain "ok" from ProtocolExchange). The protocol's own ufEnd reply also couldn't know the CRC had been compared. The session now reports the CRC only once the whole file has been written and matched (getCheckedCRC()). ProtocolExchange keeps it, by channel and file name, when the session ends, and gives it in the ufEnd reply ("crc16":"xxxx") whether the session is still there or not. A mismatch gives a ufCancel (failCRC), no crc16 in the ufEnd reply, and the file deleted - tested through ProtocolExchange. The write job no longer also logs a CRC mismatch as a failed write. Co-Authored-By: Claude Opus 5.5 --- .../FileUploadOKTOProtocol.cpp | 14 +--- .../ProtocolExchange/FileStreamSession.cpp | 10 ++- .../ProtocolExchange/FileStreamSession.h | 14 ++++ .../ProtocolExchange/ProtocolExchange.cpp | 59 ++++++++++++++- .../comms/ProtocolExchange/ProtocolExchange.h | 12 ++++ .../test_file_upload_background.cpp | 21 ++++-- linux_unit_tests/test_protocol_exchange.cpp | 72 +++++++++++++++++++ 7 files changed, 178 insertions(+), 24 deletions(-) diff --git a/components/comms/FileStreamProtocols/FileUploadOKTOProtocol.cpp b/components/comms/FileStreamProtocols/FileUploadOKTOProtocol.cpp index 572809e0..ec534c13 100644 --- a/components/comms/FileStreamProtocols/FileUploadOKTOProtocol.cpp +++ b/components/comms/FileStreamProtocols/FileUploadOKTOProtocol.cpp @@ -412,18 +412,8 @@ RaftRetCode FileUploadOKTOProtocol::handleEndMsg(const RICRESTMsg& ricRESTReqMs if (_fileStreamCancelEnd) _fileStreamCancelEnd(true); - // Response - with the CRC the file was checked against, if the client gave one (a file that didn't match - // ended the upload with a cancel, failCRC, before this) - if (_isUploading && _expCRC16Valid && (_fileStreamContentType != FILE_STREAM_CONTENT_TYPE_FIRMWARE)) - { - char crcJson[30]; - snprintf(crcJson, sizeof(crcJson), R"("crc16":"%04x")", (unsigned)(_expCRC16 & 0xffff)); - Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true, crcJson); - } - else - { - Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true); - } + // Response + Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true); // Debug #ifdef DEBUG_RICREST_FILEUPLOAD diff --git a/components/comms/ProtocolExchange/FileStreamSession.cpp b/components/comms/ProtocolExchange/FileStreamSession.cpp index d3e7ba83..150e23e9 100644 --- a/components/comms/ProtocolExchange/FileStreamSession.cpp +++ b/components/comms/ProtocolExchange/FileStreamSession.cpp @@ -699,7 +699,8 @@ void FileStreamSession::writeJob() if (rslt != RAFT_OK) { #ifdef WARN_ON_BACKGROUND_WRITE_FAILED - LOG_W(MODULE_PREFIX, "writeJob write failed filename %s", _fileStreamName.c_str()); + if (rslt != RAFT_CRC_MISMATCH) + LOG_W(MODULE_PREFIX, "writeJob write failed filename %s", _fileStreamName.c_str()); #endif RaftMutex_lock(_writeMutex, RAFT_MUTEX_WAIT_FOREVER); _writeError = rslt; @@ -719,8 +720,13 @@ RaftRetCode FileStreamSession::checkWrittenCRC(const uint8_t* pBuf, uint32_t len return RAFT_OK; if (pBuf && (len > 0)) _crcWritten = MiniHDLC::crcUpdateCCITT(_crcWritten, pBuf, len); - if (!isFinal || (_crcWritten == _crcExpected)) + if (!isFinal) return RAFT_OK; + if (_crcWritten == _crcExpected) + { + _crcMatched = true; + return RAFT_OK; + } LOG_W(MODULE_PREFIX, "upload CRC mismatch filename %s expected %04x written %04x - file deleted", _fileStreamName.c_str(), _crcExpected, _crcWritten); fileSystem.deleteFile("", _fileStreamName); diff --git a/components/comms/ProtocolExchange/FileStreamSession.h b/components/comms/ProtocolExchange/FileStreamSession.h index 74ea8b8d..fb626879 100644 --- a/components/comms/ProtocolExchange/FileStreamSession.h +++ b/components/comms/ProtocolExchange/FileStreamSession.h @@ -169,8 +169,22 @@ class FileStreamSession bool _crcCheck = false; uint16_t _crcExpected = 0; uint16_t _crcWritten = 0; + std::atomic _crcMatched{false}; RaftRetCode checkWrittenCRC(const uint8_t* pBuf, uint32_t len, bool isFinal); +public: + // The CRC16 the uploaded file was checked against - false unless it was given, the whole file has been written + // and it matched + bool getCheckedCRC(uint16_t& crc) const + { + if (!_crcMatched.load()) + return false; + crc = _crcExpected; + return true; + } + +private: + // Background reads (file downloads): a block is read on the file system worker and the next one read ahead. // fileStreamBlockRead() returns RAFT_BUSY until the block asked for has been read. The job sets the _read... // results, then clears _readJobOutstanding; the main task only uses them when no job is outstanding diff --git a/components/comms/ProtocolExchange/ProtocolExchange.cpp b/components/comms/ProtocolExchange/ProtocolExchange.cpp index c0b27042..1de0aada 100644 --- a/components/comms/ProtocolExchange/ProtocolExchange.cpp +++ b/components/comms/ProtocolExchange/ProtocolExchange.cpp @@ -94,6 +94,18 @@ void ProtocolExchange::loop() _lastUploadWriteStatus.push_back({pSession->getChannelID(), writeStatus}); if (_lastUploadWriteStatus.size() > LAST_UPLOAD_WRITE_STATUS_MAX) _lastUploadWriteStatus.pop_front(); + + // And the CRC it was checked against, for the ufEnd reply + uint16_t checkedCRC = 0; + if (pSession->getCheckedCRC(checkedCRC)) + { + _checkedUploadCRCs.remove_if([pSession](const CheckedUploadCRC& entry) { + return entry.channelID == pSession->getChannelID(); + }); + _checkedUploadCRCs.push_back({pSession->getChannelID(), pSession->getFileStreamName(), checkedCRC}); + if (_checkedUploadCRCs.size() > CHECKED_UPLOAD_CRCS_MAX) + _checkedUploadCRCs.pop_front(); + } } // Tidy up inactive session @@ -515,10 +527,21 @@ RaftRetCode ProtocolExchange::processRICRESTCmdFrame(RICRESTMsg& ricRESTReqMsg, case FileStreamBase::FILE_STREAM_MSG_TYPE_UPLOAD_END: pSession = getFileStreamExistingSession(fileStreamName.c_str(), channelID, streamID); // If session already cleaned up (e.g. after final block), respond OK - // so the client's numbered message gets acknowledged (avoiding timeout) + // so the client's numbered message gets acknowledged (avoiding timeout) - with the CRC the file was + // checked against, if it was if (!pSession) { - Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true); + uint16_t checkedCRC = 0; + if (takeCheckedUploadCRC(channelID, fileStreamName, checkedCRC)) + { + char crcJson[30]; + snprintf(crcJson, sizeof(crcJson), R"("crc16":"%04x")", (unsigned)checkedCRC); + Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true, crcJson); + } + else + { + Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true); + } return RAFT_OK; } break; @@ -569,7 +592,37 @@ RaftRetCode ProtocolExchange::processRICRESTCmdFrame(RICRESTMsg& ricRESTReqMsg, } // Session is valid so send message to it - return pSession->handleCmdFrame(fileStreamMsgType, ricRESTReqMsg, respMsg, endpointMsg); + RaftRetCode rslt = pSession->handleCmdFrame(fileStreamMsgType, ricRESTReqMsg, respMsg, endpointMsg); + + // A ufEnd for a file already checked against its CRC (the session ends once the final block is written, so + // this is the rarer case) says so + uint16_t checkedCRC = 0; + if ((fileStreamMsgType == FileStreamBase::FILE_STREAM_MSG_TYPE_UPLOAD_END) && (rslt == RAFT_OK) && + pSession->getCheckedCRC(checkedCRC)) + { + char crcJson[30]; + snprintf(crcJson, sizeof(crcJson), R"("crc16":"%04x")", (unsigned)checkedCRC); + Raft::setJsonBoolResult(ricRESTReqMsg.getReq().c_str(), respMsg, true, crcJson); + } + return rslt; +} + +///////////////////////////////////////////////////////////////////////////////////////////////////////////////// +// Take the CRC an upload (on this channel, of this file) was checked against, if it was +///////////////////////////////////////////////////////////////////////////////////////////////////////////////// + +bool ProtocolExchange::takeCheckedUploadCRC(uint32_t channelID, const String& fileName, uint16_t& crc) +{ + for (auto it = _checkedUploadCRCs.begin(); it != _checkedUploadCRCs.end(); ++it) + { + if ((it->channelID == channelID) && (it->fileName == fileName)) + { + crc = it->crc; + _checkedUploadCRCs.erase(it); + return true; + } + } + return false; } ///////////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/components/comms/ProtocolExchange/ProtocolExchange.h b/components/comms/ProtocolExchange/ProtocolExchange.h index 7cb44ee6..84874a03 100644 --- a/components/comms/ProtocolExchange/ProtocolExchange.h +++ b/components/comms/ProtocolExchange/ProtocolExchange.h @@ -79,6 +79,18 @@ class ProtocolExchange : public RaftSysMod std::list> _lastUploadWriteStatus; static const uint32_t LAST_UPLOAD_WRITE_STATUS_MAX = 4; + // Uploads checked against the CRC16 the client gave (by channel and file name), kept after the session ends + // so the ufEnd reply (which usually comes after it) can say the file was checked + struct CheckedUploadCRC + { + uint32_t channelID; + String fileName; + uint16_t crc; + }; + std::list _checkedUploadCRCs; + static const uint32_t CHECKED_UPLOAD_CRCS_MAX = 4; + bool takeCheckedUploadCRC(uint32_t channelID, const String& fileName, uint16_t& crc); + // Activity last reported through the file stream activity hook (to keep SysManager up-to-date) bool _activityReportedMainFWUpdate = false; bool _activityReportedFileSystem = false; diff --git a/linux_unit_tests/test_file_upload_background.cpp b/linux_unit_tests/test_file_upload_background.cpp index 38cf1d25..be0892e5 100644 --- a/linux_unit_tests/test_file_upload_background.cpp +++ b/linux_unit_tests/test_file_upload_background.cpp @@ -7,7 +7,7 @@ // runPending(), so each step can be checked: blocks waiting, written, the acks held back and sent, the file closed // after the final block, the session ending only then, and a cancelled upload dropping what wasn't written. An // upload given a CRC16 in ufStart is checked against it once written: a mismatch cancels it (failCRC) and deletes -// the file, and a match is confirmed in the ufEnd reply. A file +// the file, and a match is reported by getCheckedCRC() (ProtocolExchange gives it in the ufEnd reply). A file // download's blocks are read on the worker too (RAFT_BUSY until read), each one reading the next ahead. // ///////////////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -257,18 +257,22 @@ static void testCRCChecked() { printf("Running testCRCChecked...\n"); - // The right CRC: the upload completes and ufEnd confirms the CRC checked + // The right CRC: the upload completes and the session reports the CRC checked { Upload probe(12 * BLOCK - 30, "bg_upload_crc.bin"); String crc = probe.contentsCRC(); delete probe.pSession; probe.pSession = nullptr; Upload up(12 * BLOCK - 30, "bg_upload_crc.bin", crc.c_str()); + uint16_t checked = 0; + TEST_ASSERT(!up.pSession->getCheckedCRC(checked), "not checked before it is written"); TEST_ASSERT(up.sendAll().empty(), "no cancel with the right CRC"); TEST_ASSERT(up.fileContents() == up.contents, "file written"); - String endResp = up.sendEnd(); - TEST_ASSERT(RaftJson(endResp).getString("rslt", "") == "ok", "ufEnd ok"); - TEST_ASSERT(String("0x") + RaftJson(endResp).getString("crc16", "") == crc, "ufEnd gives the CRC checked"); + char checkedStr[10]; + TEST_ASSERT(up.pSession->getCheckedCRC(checked), "checked once written"); + snprintf(checkedStr, sizeof(checkedStr), "0x%04x", checked); + TEST_ASSERT(crc == checkedStr, "the CRC given"); + TEST_ASSERT(RaftJson(up.sendEnd()).getString("rslt", "") == "ok", "ufEnd ok"); } // A wrong CRC: cancelled once the final block is written (failCRC), and the file deleted @@ -277,13 +281,16 @@ static void testCRCChecked() std::vector cancels = up.sendAll(); TEST_ASSERT(cancels == std::vector({"failCRC"}), "cancelled with failCRC"); TEST_ASSERT(up.fileContents().empty(), "file deleted"); + uint16_t checked = 0; + TEST_ASSERT(!up.pSession->getCheckedCRC(checked), "no checked CRC after a mismatch"); } - // No CRC given: no check, and none in the ufEnd reply + // No CRC given: no check { Upload up(3 * BLOCK, "bg_upload_crc.bin"); TEST_ASSERT(up.sendAll().empty(), "no cancel without a CRC"); - TEST_ASSERT(RaftJson(up.sendEnd()).getString("crc16", "none") == "none", "no CRC in ufEnd"); + uint16_t checked = 0; + TEST_ASSERT(!up.pSession->getCheckedCRC(checked), "nothing checked without a CRC"); } } diff --git a/linux_unit_tests/test_protocol_exchange.cpp b/linux_unit_tests/test_protocol_exchange.cpp index ccd61f99..6c6a4c69 100644 --- a/linux_unit_tests/test_protocol_exchange.cpp +++ b/linux_unit_tests/test_protocol_exchange.cpp @@ -24,6 +24,10 @@ #include "RestAPIEndpointManager.h" #include "SysManagerIF.h" #include "FileStreamBlock.h" +#include "FileSystem.h" +#include "FileSystemWorker.h" +#include "MiniHDLC.h" +#include static const uint32_t TEST_CHANNEL_ID = 7; static const char* TEST_STREAM_ENDPOINT = "teststream"; @@ -340,11 +344,79 @@ static void testDownloadOfMissingFileRefused() RaftSysMod::setSysManager(nullptr); } +// Uploads with a CRC16 in ufStart: the session ends once the final block is written and checked, and the ufEnd +// that follows says the file was checked (crc16), or the upload is cancelled (failCRC) if it doesn't match +static void testUploadCRCInEndReply() +{ + printf("Running testUploadCRCInEndReply...\n"); + + TestCommsCore commsCore; + RestAPIEndpointManager restAPI; + TestSysManager sysManager(commsCore, restAPI); + RaftSysMod::setSysManager(&sysManager); + fileSystem.setup(FileSystem::LOCAL_FS_SPIFFS, false, false, -1, -1, -1, -1, false, false, false); + fileSystemWorker.setRunAtOnce(true); + + RaftJson config("{}"); + ProtocolExchange protocolExchange("ProtExchg", config); + RaftSysMod& sysMod = protocolExchange; + sysMod.addCommsChannels(commsCore); + if (!commsCore.ricSerialHandler) + { + TEST_ASSERT(false, "RICSerial message handler registered"); + return; + } + + char cwd[512]; + String path = String(getcwd(cwd, sizeof(cwd))) + "/px_upload_crc.bin"; + std::vector data(300); + for (uint32_t i = 0; i < data.size(); i++) + data[i] = (uint8_t)(i * 13); + char crcStr[10]; + snprintf(crcStr, sizeof(crcStr), "0x%04x", MiniHDLC::crcUpdateCCITT(MiniHDLC::crcInitCCITT(), data.data(), data.size())); + + // Upload with `crc` in ufStart; returns the ufEnd reply, and the reasons of any cancels + auto upload = [&](const char* crc, std::vector& cancels) -> RaftJson { + commsCore.responses.clear(); + unlink(path.c_str()); + sendCmdFrame(commsCore, 1, String(R"({"cmdName":"ufStart","reqStr":"fileupload","fileType":"fs","fileName":")") + + path + R"(","fileLen":300,"batchMsgSize":300,"batchAckSize":1,"CRC16":")" + crc + "\"}"); + uint32_t streamID = commsCore.responses.empty() ? 0 : RaftJson(commsCore.responses[0]).getLong("streamID", 0); + sendFileBlock(commsCore, 0, streamID, 0, data); + for (int i = 0; i < 5; i++) + sysMod.loop(); + for (const String& resp : commsCore.responses) + if (RaftJson(resp).getString("cmdName", "") == "ufCancel") + cancels.push_back(RaftJson(resp).getString("reason", "")); + commsCore.responses.clear(); + sendCmdFrame(commsCore, 2, String(R"({"cmdName":"ufEnd","reqStr":"fileupload","fileType":"fs","fileName":")") + + path + R"(","fileLen":300,"streamID":)" + String(streamID) + "}"); + return RaftJson(commsCore.responses.empty() ? String("{}") : commsCore.responses.back()); + }; + + std::vector cancels; + RaftJson endResp = upload(crcStr, cancels); + TEST_ASSERT(cancels.empty(), "no cancel with the right CRC"); + TEST_ASSERT(endResp.getString("rslt", "") == "ok", "ufEnd ok"); + TEST_ASSERT(String("0x") + endResp.getString("crc16", "") == crcStr, "ufEnd gives the CRC the file was checked against"); + + cancels.clear(); + endResp = upload("0x1234", cancels); + TEST_ASSERT(cancels == std::vector({"failCRC"}), "a wrong CRC cancels the upload (failCRC)"); + TEST_ASSERT(endResp.getString("crc16", "none") == "none", "no CRC in the ufEnd reply after a mismatch"); + TEST_ASSERT(access(path.c_str(), F_OK) != 0, "the file is deleted"); + + unlink(path.c_str()); + fileSystemWorker.setRunAtOnce(false); + RaftSysMod::setSysManager(nullptr); +} + int main() { testStreamSessionAndActivityHook(); testStreamToUnknownEndpointRefused(); testConcurrentSessionsReportedTogether(); testDownloadOfMissingFileRefused(); + testUploadCRCInEndReply(); return testSummary("test_protocol_exchange"); }