[ota-provider-app] fix location parsing and BDX sender (#9733)

* ota-provider fixes * add utility HasMessageType() and remove old comments
project-chip · Sep 30, 2021 · 1988706 · 1988706
1 parent bc32734
commit 1988706
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 8 deletions.
diff --git a/examples/ota-provider-app/ota-provider-common/BdxOtaSender.cpp b/examples/ota-provider-app/ota-provider-common/BdxOtaSender.cpp
@@ -21,6 +21,7 @@
 #include <lib/support/BitFlags.h>
 #include <lib/support/CHIPMemString.h>
 #include <messaging/ExchangeContext.h>
+#include <messaging/Flags.h>
 #include <protocols/bdx/BdxTransferSession.h>
 
 #include <fstream>
@@ -59,14 +60,23 @@ void BdxOtaSender::HandleTransferSessionOutput(TransferSession::OutputEvent & ev
     {
     case TransferSession::OutputEventType::kNone:
         break;
-    case TransferSession::OutputEventType::kMsgToSend:
+    case TransferSession::OutputEventType::kMsgToSend: {
+        chip::Messaging::SendFlags sendFlags;
+        if (!event.msgTypeData.HasMessageType(chip::Protocols::SecureChannel::MsgType::StatusReport))
+        {
+            // All messages sent from the Sender expect a response, except for a StatusReport which would indicate an error and the
+            // end of the transfer.
+            sendFlags.Set(chip::Messaging::SendMessageFlags::kExpectResponse);
+        }
         VerifyOrReturn(mExchangeCtx != nullptr, ChipLogError(BDX, "%s: mExchangeCtx is null", __FUNCTION__));
-        err = mExchangeCtx->SendMessage(event.msgTypeData.ProtocolId, event.msgTypeData.MessageType, std::move(event.MsgData));
+        err = mExchangeCtx->SendMessage(event.msgTypeData.ProtocolId, event.msgTypeData.MessageType, std::move(event.MsgData),
+                                        sendFlags);
         if (err != CHIP_NO_ERROR)
         {
             ChipLogError(BDX, "SendMessage failed: %s", chip::ErrorStr(err));
         }
         break;
+    }
     case TransferSession::OutputEventType::kInitReceived: {
         // TransferSession will automatically reject a transfer if there are no
         // common supported control modes. It will also default to the smaller
@@ -97,19 +107,20 @@ void BdxOtaSender::HandleTransferSessionOutput(TransferSession::OutputEvent & ev
         {
             // TODO: AbortTransfer() needs to support GeneralStatusCode failures as well as BDX specific errors.
             mTransfer.AbortTransfer(StatusCode::kUnknown);
+            return;
         }
 
         std::ifstream otaFile(mFilepath, std::ifstream::in);
         VerifyOrReturn(otaFile.good(), ChipLogError(BDX, "%s: file read failed", __FUNCTION__));
         otaFile.seekg(mNumBytesSent);
         otaFile.read(reinterpret_cast<char *>(blockBuf->Start()), bytesToRead);
-        VerifyOrReturn(otaFile.good(), ChipLogError(BDX, "%s: file read failed", __FUNCTION__));
+        VerifyOrReturn(otaFile.good() || otaFile.eof(), ChipLogError(BDX, "%s: file read failed", __FUNCTION__));
 
         blockData.Data   = blockBuf->Start();
         blockData.Length = otaFile.gcount();
-        blockData.IsEof  = (otaFile.gcount() < blockSize) ||
-            (mNumBytesSent + static_cast<uint64_t>(otaFile.gcount()) == mTransfer.GetTransferLength() || (otaFile.peek() == EOF));
-        mNumBytesSent = static_cast<uint32_t>(mNumBytesSent + otaFile.gcount());
+        blockData.IsEof  = (blockData.Length < blockSize) ||
+            (mNumBytesSent + static_cast<uint64_t>(blockData.Length) == mTransfer.GetTransferLength() || (otaFile.peek() == EOF));
+        mNumBytesSent = static_cast<uint32_t>(mNumBytesSent + blockData.Length);
 
         VerifyOrReturn(CHIP_NO_ERROR == mTransfer.PrepareBlock(blockData),
                        ChipLogError(BDX, "%s: PrepareBlock failed: %s", __FUNCTION__, chip::ErrorStr(err)));

diff --git a/src/app/clusters/ota-provider/ota-provider.cpp b/src/app/clusters/ota-provider/ota-provider.cpp
@@ -168,10 +168,10 @@ bool emberAfOtaSoftwareUpdateProviderClusterQueryImageCallback(EndpointId endpoi
     ChipLogDetail(Zcl, "OTA Provider received QueryImage");
 
     // TODO: (#7112) change location size checking once CHAR_STRING is supported
-    const uint8_t locationLen = emberAfStringLength(location);
+    const size_t locationLen = strlen(reinterpret_cast<char *>(location));
     if (locationLen != kLocationParamLength)
     {
-        ChipLogError(Zcl, "expected location length %" PRIu8 ", got %" PRIu8, locationLen, kLocationParamLength);
+        ChipLogError(Zcl, "expected location length %" PRIu8 ", got %zu", kLocationParamLength, locationLen);
         emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_INVALID_ARGUMENT);
     }
     else if (metadataForProvider.size() > kMaxMetadataLen)

diff --git a/src/protocols/bdx/BdxTransferSession.h b/src/protocols/bdx/BdxTransferSession.h
@@ -12,6 +12,8 @@
 #include <system/SystemPacketBuffer.h>
 #include <transport/raw/MessageHeader.h>
 
+#include <type_traits>
+
 namespace chip {
 namespace bdx {
 
@@ -86,6 +88,14 @@ class DLL_EXPORT TransferSession
         uint8_t MessageType;
 
         MessageTypeData() : ProtocolId(Protocols::NotSpecified), MessageType(0) {}
+
+        bool HasProtocol(Protocols::Id protocol) const { return ProtocolId == protocol; }
+        bool HasMessageType(uint8_t type) const { return MessageType == type; }
+        template <typename TMessageType, typename = std::enable_if_t<std::is_enum<TMessageType>::value>>
+        bool HasMessageType(TMessageType type) const
+        {
+            return HasProtocol(Protocols::MessageTypeTraits<TMessageType>::ProtocolId()) && HasMessageType(to_underlying(type));
+        }
     };
 
     /**