Skip to content

[nrf toup][nrfconnect] Optimize OTA time #707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kkasperczyk-no merged 1 commit into from
Mar 18, 2026
Merged

[nrf toup][nrfconnect] Optimize OTA time #707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 64 additions & 35 deletions src/platform/nrfconnect/OTAImageProcessorImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

#include <app/clusters/ota-requestor/OTADownloader.h>
#include <app/clusters/ota-requestor/OTARequestorInterface.h>
#include <cstring>
#include <lib/support/CodeUtils.h>
#include <platform/CHIPDeviceLayer.h>
#include <system/SystemError.h>
Expand Down Expand Up @@ -126,20 +127,29 @@ CHIP_ERROR OTAImageProcessorImpl::PrepareDownloadImpl()

CHIP_ERROR OTAImageProcessorImpl::Finalize()
{
PostOTAStateChangeEvent(DeviceLayer::kOtaDownloadComplete);
DFUSync::GetInstance().Free(mDfuSyncMutexId);
return System::MapErrorZephyr(dfu_multi_image_done(true));
return DeviceLayer::SystemLayer().ScheduleLambda([this] {
PostOTAStateChangeEvent(DeviceLayer::kOtaDownloadComplete);
DFUSync::GetInstance().Free(mDfuSyncMutexId);
CHIP_ERROR error = System::MapErrorZephyr(dfu_multi_image_done(true));
if (error != CHIP_NO_ERROR)
{
ChipLogError(SoftwareUpdate, "OTA failed to finalize: %" CHIP_ERROR_FORMAT, error.Format());
}
});
}

CHIP_ERROR OTAImageProcessorImpl::Abort()
{
CHIP_ERROR error = System::MapErrorZephyr(dfu_multi_image_done(false));

DFUSync::GetInstance().Free(mDfuSyncMutexId);
TriggerFlashAction(ExternalFlashManager::Action::SLEEP);
PostOTAStateChangeEvent(DeviceLayer::kOtaDownloadAborted);

return error;
return DeviceLayer::SystemLayer().ScheduleLambda([this] {
CHIP_ERROR error = System::MapErrorZephyr(dfu_multi_image_done(false));
DFUSync::GetInstance().Free(mDfuSyncMutexId);
TriggerFlashAction(ExternalFlashManager::Action::SLEEP);
PostOTAStateChangeEvent(DeviceLayer::kOtaDownloadAborted);
if (error != CHIP_NO_ERROR)
{
ChipLogError(SoftwareUpdate, "Failed to abort OTA: %" CHIP_ERROR_FORMAT, error.Format());
}
});
}

CHIP_ERROR OTAImageProcessorImpl::Apply()
Expand All @@ -162,48 +172,67 @@ CHIP_ERROR OTAImageProcessorImpl::Apply()
},
nullptr /* context */);
}
else
{
PostOTAStateChangeEvent(DeviceLayer::kOtaApplyFailed);
return System::MapErrorZephyr(err);
}
PostOTAStateChangeEvent(DeviceLayer::kOtaApplyFailed);
return System::MapErrorZephyr(err);
#else
return System::MapErrorZephyr(err);
#endif
}

CHIP_ERROR OTAImageProcessorImpl::WriteToFlash(size_t offset, const uint8_t * chunk, size_t chunk_size)
{
int err = dfu_multi_image_write(offset, chunk, chunk_size);
if (err != 0)
{
ChipLogError(SoftwareUpdate, "OTA block write failed %d", err);
return CHIP_ERROR_WRITE_FAILED;
}
mParams.downloadedBytes += chunk_size;
ChipLogDetail(SoftwareUpdate, "Downloaded %u/%u bytes", static_cast<unsigned>(mParams.downloadedBytes),
static_cast<unsigned>(mParams.totalFileBytes));
return CHIP_NO_ERROR;
}

CHIP_ERROR OTAImageProcessorImpl::ProcessBlock(ByteSpan & aBlock)
{
VerifyOrReturnError(mDownloader != nullptr, CHIP_ERROR_INCORRECT_STATE);

CHIP_ERROR error = ProcessHeader(aBlock);

if (error == CHIP_NO_ERROR)
const size_t blockOffset = static_cast<size_t>(mParams.downloadedBytes);
const size_t blockSize = aBlock.size();

if (error == CHIP_NO_ERROR && blockSize > kBufferSize)
{
// DFU target library buffers data internally, so do not clone the block data.
if (mParams.downloadedBytes > std::numeric_limits<size_t>::max())
{
error = CHIP_ERROR_BUFFER_TOO_SMALL;
}
else
{
error = System::MapErrorZephyr(
dfu_multi_image_write(static_cast<size_t>(mParams.downloadedBytes), aBlock.data(), aBlock.size()));
mParams.downloadedBytes += aBlock.size();
}
error = CHIP_ERROR_BUFFER_TOO_SMALL;
}

if (error != CHIP_NO_ERROR)
{
DeviceLayer::SystemLayer().ScheduleLambda([this, error] {
mDownloader->EndDownload(error);
PostOTAStateChangeEvent(DeviceLayer::kOtaDownloadFailed);
});
return error;
}

// Report the result back to the downloader asynchronously.
return DeviceLayer::SystemLayer().ScheduleLambda([this, error, aBlock] {
if (error == CHIP_NO_ERROR)
memcpy(mStagingBuffer, aBlock.data(), blockSize);

return DeviceLayer::SystemLayer().ScheduleLambda([this, blockOffset, blockSize] {
CHIP_ERROR err = CHIP_NO_ERROR;
const bool isLastBlock = blockOffset + blockSize >= mParams.totalFileBytes;
if (!isLastBlock)
Copy link
Copy Markdown
Contributor

@Damian-Nordic Damian-Nordic Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have time to check this, but it's surprising that this is needed as TransferSession::PrepareBlockQuery() should exit early due to VerifyOrReturnError(mState == TransferState::kTransferInProgress, CHIP_ERROR_INCORRECT_STATE); after the last block has been sent. But anyway, there's probably no harm to skip it either, so it's OK :).

Copy link
Copy Markdown
Contributor Author

@Adam-Maciuga Adam-Maciuga Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some investigation to understand why this was not needed but is now:
Flow of program before (last block):

  1. Process block flashes the block
  2. Lambda requesting the next block is scheduled
  3. Finalize is called (synchronous)
  4. The lambda is executed; there was no error handling for mDownloader->FetchNextData();

Now:

  1. Process block schedules the lambda
  2. Finalize schedules its lambda
  3. Process Block lambda runs, mDownloader->FetchNextData() returns an error (expected since the download is finished), The error is handled and the mDownloader->EndDownload(); is called
  4. the download process doesn't finish properly

Therefore, the last block has to be processed a bit differently

Copy link
Copy Markdown
Contributor

@Damian-Nordic Damian-Nordic Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so it worked only because FetchNextData error wasn't checked. Thanks for checking :)

{
ChipLogDetail(SoftwareUpdate, "Downloaded %u/%u bytes", static_cast<unsigned>(mParams.downloadedBytes),
static_cast<unsigned>(mParams.totalFileBytes));
mDownloader->FetchNextData();
err = mDownloader->FetchNextData();
}
else
if (err == CHIP_NO_ERROR)
{
mDownloader->EndDownload(error);
err = WriteToFlash(blockOffset, mStagingBuffer, blockSize);
}
if (err != CHIP_NO_ERROR)
{
ChipLogError(SoftwareUpdate, "OTA block processing failed: %" CHIP_ERROR_FORMAT, err.Format());
mDownloader->EndDownload(err);
PostOTAStateChangeEvent(DeviceLayer::kOtaDownloadFailed);
}
});
Expand Down
5 changes: 5 additions & 0 deletions src/platform/nrfconnect/OTAImageProcessorImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,17 @@ class OTAImageProcessorImpl : public OTAImageProcessorInterface
protected:
CHIP_ERROR PrepareDownloadImpl();
CHIP_ERROR ProcessHeader(ByteSpan & aBlock);
CHIP_ERROR WriteToFlash(size_t offset, const uint8_t * chunk, size_t chunk_size);

OTADownloader * mDownloader = nullptr;
OTAImageHeaderParser mHeaderParser;
uint8_t mBuffer[kBufferSize];
ExternalFlashManager * mFlashHandler;

/** When new data is fetched it is copied to the staging buffer,
* this way we can simultaneously receive the next chunk and store the old one to flash */
uint8_t mStagingBuffer[kBufferSize];

private:
bool mImageConfirmed = false;
uint32_t mDfuSyncMutexId;
Expand Down
Loading