cleaning up

examples/ota-requestor-app/esp32/main/OTARequesterImpl.cpp

@@ -72,7 +72,7 @@ enum OTARequestorCommands
 namespace {
 // TODO: Encapsulate these globals and the callbacks in some class
 ExchangeContext * exchangeCtx = nullptr;
-BDXDownloader bdxDownloader;
+BdxDownloader bdxDownloader;
 enum OTARequestorCommands operationalDeviceContext;
 
 constexpr uint8_t kMaxUpdateTokenLen       = 32; // must be between 8 and 32

examples/ota-requestor-app/linux/LinuxOTAImageProcessor.cpp

@@ -97,6 +97,8 @@ void LinuxOTAImageProcessor::HandlePrepareDownload(intptr_t context)
         return;
     }
 
+    // TODO: if file already exists and is not empty, erase previous contents
+
     imageProcessor->mDownloader->OnPreparedForDownload(CHIP_NO_ERROR);
 }
 
@@ -110,6 +112,8 @@ void LinuxOTAImageProcessor::HandleFinalize(intptr_t context)
 
     imageProcessor->mOfs.close();
     imageProcessor->ReleaseBlock();
+
+    ChipLogProgress(SoftwareUpdate, "OTA image downloaded to %s", imageProcessor->mParams.imageFile.data());
 }
 
 void LinuxOTAImageProcessor::HandleAbort(intptr_t context)

examples/ota-requestor-app/linux/main.cpp

@@ -208,7 +208,10 @@ int main(int argc, char * argv[])
     // Connect the Requestor and Requestor Driver objects
     requestorCore.SetOtaRequestorDriver(&requestorUser);
 
-    // Initialize the Image Processor object
+    // WARNING: this is probably not realistic to know such details of the image or to even have an OTADownloader instantiated at
+    // the beginning of program execution. We're using hardcoded values here for now since this is a reference application.
+    // TODO: instatiate and initialize these values when QueryImageResponse tells us an image is available
+    // TODO: add API for OTARequestor to pass QueryImageResponse info to the application to use for OTADownloader init
     OTAImageProcessorParams ipParams;
     ipParams.imageFile = CharSpan("test.txt");
     imageProcessor.SetOTAImageProcessorParams(ipParams);

src/app/clusters/ota-requestor/BDXDownloader.cpp

@@ -1,8 +1,29 @@
+/*
+ *
+ *    Copyright (c) 2021 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
 #include "BDXDownloader.h"
 
 #include <lib/core/CHIPError.h>
 #include <lib/support/CodeUtils.h>
 #include <protocols/bdx/BdxMessages.h>
+#include <system/SystemClock.h> /* TODO:(#12520) remove */
+#include <system/SystemPacketBuffer.h>
+#include <transport/raw/MessageHeader.h>
 
 using chip::OTADownloader;
 using chip::bdx::TransferSession;
@@ -18,14 +39,18 @@ void BDXDownloader::OnMessageReceived(const chip::PayloadHeader & payloadHeader,
     {
         ChipLogError(BDX, "unable to handle message: %" CHIP_ERROR_FORMAT, err.Format());
     }
+
+    // HandleMessageReceived() will only decode/parse the message. Need to Poll() in order to do the message handling work in
+    // HandleBdxEvent().
     PollTransferSession();
 }
 
 CHIP_ERROR BDXDownloader::SetBDXParams(const chip::bdx::TransferSession::TransferInitData & bdxInitData)
 {
     VerifyOrReturnError(mState == State::kIdle, CHIP_ERROR_INCORRECT_STATE);
 
-    // Must call StartTransfer() here or otherwise the pointer data contained in bdxInitData could be freed before we can use it.
+    // Must call StartTransfer() here to store the the pointer data contained in bdxInitData in the TransferSession object.
+    // Otherwise it could be freed before we can use it.
     ReturnErrorOnFailure(mBdxTransfer.StartTransfer(chip::bdx::TransferRole::kReceiver, bdxInitData,
                                                     /* TODO:(#12520) */ chip::System::Clock::Seconds16(30)));
 
@@ -50,6 +75,8 @@ CHIP_ERROR BDXDownloader::OnPreparedForDownload(CHIP_ERROR status)
     if (status == CHIP_NO_ERROR)
     {
         mState = State::kInProgress;
+
+        // Must call here because StartTransfer() should have prepared a ReceiveInit message, and now we should send it.
         PollTransferSession();
     }
     else
@@ -75,7 +102,13 @@ void BDXDownloader::OnDownloadTimeout()
 {
     if (mState == State::kInProgress)
     {
-        mBdxTransfer.AbortTransfer(chip::bdx::StatusCode::kUnknown);
+        ChipLogDetail(BDX, "aborting due to timeout");
+        mBdxTransfer.Reset();
+        if (mImageProcessor != nullptr)
+        {
+            mImageProcessor->Abort();
+        }
+        mState = State::kIdle;
     }
     else
     {
@@ -87,26 +120,29 @@ void BDXDownloader::EndDownload(CHIP_ERROR reason)
 {
     if (mState == State::kInProgress)
     {
+        // There's no method for a BDX receiving driver to cleanly end a transfer
         mBdxTransfer.AbortTransfer(chip::bdx::StatusCode::kUnknown);
         if (mImageProcessor != nullptr)
         {
             mImageProcessor->Abort();
         }
+        mState = State::kIdle;
+
+        // Because AbortTransfer() will generate a StatusReport to send.
+        PollTransferSession();
     }
     else
     {
         ChipLogError(BDX, "no download in progress");
     }
-
-    // Because AbortTransfer() will generate a StatusReport to send.
-    PollTransferSession();
 }
 
 void BDXDownloader::PollTransferSession()
 {
     TransferSession::OutputEvent outEvent;
 
-    // TODO: Is this dangerous? What happens if the loop encounters two messages that need to be sent?
+    // WARNING: Is this dangerous? What happens if the loop encounters two messages that need to be sent? Does the ExchangeContext
+    // allow that?
     do
     {
         mBdxTransfer.PollOutput(outEvent, /* TODO:(#12520) */ chip::System::Clock::Seconds16(0));
@@ -117,7 +153,6 @@ void BDXDownloader::PollTransferSession()
 
 CHIP_ERROR BDXDownloader::HandleBdxEvent(const chip::bdx::TransferSession::OutputEvent & outEvent)
 {
-    VerifyOrReturnError(mState == State::kInProgress, CHIP_ERROR_INCORRECT_STATE);
     VerifyOrReturnError(mImageProcessor != nullptr, CHIP_ERROR_INCORRECT_STATE);
 
     switch (outEvent.EventType)
@@ -126,14 +161,17 @@ CHIP_ERROR BDXDownloader::HandleBdxEvent(const chip::bdx::TransferSession::Outpu
         break;
     case TransferSession::OutputEventType::kAcceptReceived:
         ReturnErrorOnFailure(mBdxTransfer.PrepareBlockQuery());
-        // TODO: need to check ReceiveAccept parameters?
+        // TODO: need to check ReceiveAccept parameters
         break;
     case TransferSession::OutputEventType::kMsgToSend: {
         VerifyOrReturnError(mMsgDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE);
         ReturnErrorOnFailure(mMsgDelegate->SendMessage(outEvent));
         if (outEvent.msgTypeData.HasMessageType(chip::bdx::MessageType::BlockAckEOF))
         {
+            // BDX transfer is not complete until BlockAckEOF has been sent
             mState = State::kComplete;
+
+            // TODO: how/when to reset the BDXDownloader to be ready to handle another download
         }
         break;
     }

src/app/clusters/ota-requestor/BDXDownloader.h

@@ -16,13 +16,18 @@
  *    limitations under the License.
  */
 
+/* This file contains a definition for a class that implements OTADownloader and downloads CHIP OTA images using the BDX protocol.
+ * It should not execute any logic that is application specific.
+ */
+
+// TODO: unit tests
+
 #pragma once
 
 #include "OTADownloader.h"
 
 #include <lib/core/CHIPError.h>
 #include <protocols/bdx/BdxTransferSession.h>
-#include <system/SystemClock.h>
 #include <system/SystemPacketBuffer.h>
 #include <transport/raw/MessageHeader.h>
 
@@ -31,6 +36,8 @@ namespace chip {
 class BDXDownloader : public chip::OTADownloader
 {
 public:
+    // A delegate for passing messages to/from BDXDownloader and some messaging layer. This is mainly to make BDXDownloader more
+    // easily unit-testable.
     class MessagingDelegate
     {
     public:
@@ -40,7 +47,9 @@ class BDXDownloader : public chip::OTADownloader
 
     BDXDownloader() : chip::OTADownloader() {}
 
+    // To be called when there is an incoming message to handle (of any protocol type)
     void OnMessageReceived(const chip::PayloadHeader & payloadHeader, chip::System::PacketBufferHandle msg);
+
     void SetMessageDelegate(MessagingDelegate * delegate) { mMsgDelegate = delegate; }
 
     // Initialize a BDX transfer session but will not proceed until OnPreparedForDownload() is called.

src/app/clusters/ota-requestor/OTADownloader.h

@@ -61,7 +61,7 @@ class OTADownloader
     // The reason parameter should be used to indicate if this is a graceful end or a forceful abort.
     void virtual EndDownload(CHIP_ERROR reason = CHIP_NO_ERROR) = 0;
 
-    // Fetch the next set of data. May not make sense for asynchronous protocols.
+    // Fetch the next set of data. May be a no-op for asynchronous protocols.
     CHIP_ERROR virtual FetchNextData() { return CHIP_ERROR_NOT_IMPLEMENTED; }
 
     // Skip ahead some number of bytes in the download of the image file. May not be supported by some transport protocols.
@@ -72,8 +72,6 @@ class OTADownloader
 
     State GetState() const { return mState; }
 
-    // API declarations end
-
     virtual ~OTADownloader() = default;
 
 protected:

src/app/clusters/ota-requestor/OTARequestor.cpp

@@ -365,6 +365,8 @@ void OTARequestor::mOnConnected(void * context, chip::DeviceProxy * deviceProxy)
     case kStartBDX: {
         VerifyOrReturn(mBdxDownloader != nullptr, ChipLogError(SoftwareUpdate, "downloader is null"));
 
+        // TODO: allow caller to provide their own OTADownloader instance and set BDX parameters
+
         TransferSession::TransferInitData initOptions;
         initOptions.TransferCtlFlags = chip::bdx::TransferControlFlags::kReceiverDrive;
         initOptions.MaxBlockSize     = 1024;

src/app/clusters/ota-requestor/OTARequestor.h

@@ -97,11 +97,12 @@ class OTARequestor : public OTARequestorInterface
             VerifyOrReturnError(mExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE);
 
             chip::Messaging::SendFlags sendFlags;
-            if (event.msgTypeData.MessageType == static_cast<uint8_t>(chip::bdx::MessageType::ReceiveInit))
+            if (event.msgTypeData.HasMessageType(chip::bdx::MessageType::ReceiveInit))
             {
                 sendFlags.Set(chip::Messaging::SendMessageFlags::kFromInitiator);
             }
-            if (event.msgTypeData.MessageType != static_cast<uint8_t>(chip::bdx::MessageType::BlockAckEOF))
+            if (!event.msgTypeData.HasMessageType(chip::bdx::MessageType::BlockAckEOF) &&
+                !event.msgTypeData.HasMessageType(chip::Protocols::SecureChannel::MsgType::StatusReport))
             {
                 sendFlags.Set(chip::Messaging::SendMessageFlags::kExpectResponse);
             }
@@ -123,6 +124,7 @@ class OTARequestor : public OTARequestorInterface
                 mDownloader->OnMessageReceived(payloadHeader, payload.Retain());
             }
 
+            // For a receiver using BDX Protocol, all received messages will require a response except for a StatusReport
             if (!payloadHeader.HasMessageType(chip::Protocols::SecureChannel::MsgType::StatusReport))
             {
                 ec->WillSendMessage();
@@ -159,8 +161,8 @@ class OTARequestor : public OTARequestorInterface
     chip::CASESessionManager * mCASESessionManager = nullptr;
     OnConnectedState onConnectedState              = kQueryImage;
     chip::Messaging::ExchangeContext * exchangeCtx = nullptr;
-    chip::BDXDownloader * mBdxDownloader;
-    BDXMessenger mBdxMessenger;
+    chip::BDXDownloader * mBdxDownloader; // TODO: this should be OTADownloader
+    BDXMessenger mBdxMessenger;           // TODO: ideally this is held by the application
 
     // TODO: Temporary until IP address resolution is implemented in the Exchange layer
     chip::Inet::IPAddress mIpAddress;